FIX: Context manager commits on clean exit, rolls back on exception#639
Open
bewithgaurav wants to merge 2 commits into
Open
FIX: Context manager commits on clean exit, rolls back on exception#639bewithgaurav wants to merge 2 commits into
bewithgaurav wants to merge 2 commits into
Conversation
7a7d0d5 to
9489153
Compare
bewithgaurav
commented
Jun 22, 2026
| return self | ||
|
|
||
| def __exit__(self, *args: Any) -> None: | ||
| def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None: |
Collaborator
Author
There was a problem hiding this comment.
why *args → exc_type, exc_val, exc_tb:
the old __exit__ didn't inspect its arguments at all (just called close()), so *args was fine. now we branch on exc_type to decide commit vs rollback, so we use the canonical __exit__(self, exc_type, exc_val, exc_tb) signature from the Python data model.
this is what sqlite3, psycopg2, and every other DB-API driver uses. no behavioral change from the rename itself - just makes the branching logic readable.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/connection.pyLines 1714-1722 1714 (rollback or close) are suppressed so the original user exception
1715 propagates unchanged.
1716 """
1717 if self._closed:
! 1718 return
1719 try:
1720 if not self.autocommit:
1721 if exc_type is None:
1722 self.commit()Lines 1720-1741 1720 if not self.autocommit:
1721 if exc_type is None:
1722 self.commit()
1723 else:
! 1724 self.rollback()
! 1725 except Exception:
! 1726 try:
! 1727 self.close()
! 1728 except Exception:
! 1729 pass
! 1730 if exc_type is None:
! 1731 raise
! 1732 return
1733 try:
1734 self.close()
! 1735 except Exception:
! 1736 if exc_type is None:
! 1737 raise
1738
1739 def __del__(self) -> None:
1740 """
1741 Destructor to ensure the connection is closed when the connection object📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection.cpp: 76.2%
mssql_python.pybind.ddbc_bindings.cpp: 76.2%
mssql_python.row.py: 76.9%
mssql_python.__init__.py: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.4%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
Fixes #635. The __exit__ implementation previously only called close(), which always rolled back uncommitted changes. This contradicted the documented behavior in the wiki, the DevBlogs post, and the docstrings. The fix adds commit-on-success / rollback-on-exception semantics to __exit__, matching the standard pattern used by sqlite3, psycopg2, and other Python DB-API drivers. close() remains unchanged; its internal rollback is a harmless no-op on an already-resolved transaction. 24 subprocess-isolated tests cover: clean commit, exception rollback, autocommit modes, manual commit/rollback inside block, DDL+DML, pending results, doomed transactions, KeyboardInterrupt, generator abandonment, killed SPID, large transactions, and double-exit idempotency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9489153 to
b3c915a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the Connection context manager behavior to match documented transaction semantics: commit on clean exit and rollback on exception when autocommit=False, while always closing the connection on exit.
Changes:
- Implemented commit-on-success / rollback-on-exception logic in
Connection.__exit__. - Added a new subprocess-isolated test suite to validate context manager transaction behavior and crash-safety edge cases.
- Marked resource-intensive context-manager tests as
@pytest.mark.stress(excluded by default viapytest.ini).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
mssql_python/connection.py |
Updates Connection.__exit__ to commit/rollback based on exit condition and autocommit, then close the connection. |
tests/test_024_context_manager_transaction.py |
Adds subprocess-based tests covering commit/rollback semantics, exception propagation, and various hardening scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+822
to
+852
| result = _run_script(""" | ||
| import os | ||
| from mssql_python import connect | ||
|
|
||
| cs = os.environ["DB_CONNECTION_STRING"] | ||
|
|
||
| setup = connect(cs) | ||
| setup.cursor().execute("IF OBJECT_ID('pytest_ctx_large') IS NOT NULL DROP TABLE pytest_ctx_large") | ||
| setup.cursor().execute("CREATE TABLE pytest_ctx_large (id INT)") | ||
| setup.commit() | ||
| setup.close() | ||
|
|
||
| with connect(cs) as conn: | ||
| cur = conn.cursor() | ||
| for i in range(10000): | ||
| cur.execute(f"INSERT INTO pytest_ctx_large VALUES ({i})") | ||
|
|
||
| verify = connect(cs) | ||
| cur = verify.cursor() | ||
| cur.execute("SELECT COUNT(*) FROM pytest_ctx_large") | ||
| count = cur.fetchone()[0] | ||
| assert count == 10000, f"FAIL: expected 10000 rows, got {count}" | ||
| verify.close() | ||
|
|
||
| cleanup = connect(cs) | ||
| cleanup.cursor().execute("DROP TABLE pytest_ctx_large") | ||
| cleanup.commit() | ||
| cleanup.close() | ||
| print("PASS") | ||
| """) | ||
| _assert_no_crash(result, "test_large_transaction_commit") |
…eoutcomments - Wrap final self.close() in try/except so cleanup failures don't mask the user's original exception on the exception path - Remove hardcoded fallback connection string from tests (tests now properly skip when DB_CONNECTION_STRING is unset) - Replace signal.Signals._value2member_map_ (private API) with try/except ValueError for stable cross-version behavior - Bump subprocess timeout to 120s for 10k-insert stress test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Fixes the Connection context manager (
__exit__) to implement commit-on-success / rollback-on-exception semantics, matching the documented behavior in the wiki, the DevBlogs post, and the docstrings.Previously
__exit__only calledclose(), which always rolled back uncommitted changes regardless of whether the block exited cleanly or via exception. Now:autocommit=False: commits the transactionautocommit=False: rolls back the transactionautocommit=True: no explicit commit/rollback (same as before)close()is unchanged. Its internal rollback becomes a harmless no-op on an already-resolved transaction.24 subprocess-isolated tests cover: commit/rollback semantics, autocommit toggling, manual commit/rollback inside block, DDL+DML, pending cursor results, doomed transactions (XACT_ABORT), KeyboardInterrupt, generator abandonment, killed SPID, 10k-row transactions, double-exit idempotency, and GC safety. Resource-intensive tests (rapid connection cycling, 10k inserts) are marked
@pytest.mark.stress.