Session is not set in object if connection fails, which raises 'AttributeError' on __del__ (#746)#877
Open
peco-engineer-bot[bot] wants to merge 2 commits into
Open
Session is not set in object if connection fails, which raises 'AttributeError' on __del__ (#746)#877peco-engineer-bot[bot] wants to merge 2 commits into
peco-engineer-bot[bot] wants to merge 2 commits into
Conversation
…buteError' on __del__ (#746) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the hasattr(self, "session") guard in Connection.__del__ correctly fixes the AttributeError, since the username/password ValueError is raised inside Session.__init__ (session.py:121 → auth.py:109) before self.session is assigned. One low note: the regression test is E2E-only despite covering purely client-side behavior that could be unit-tested.
Addresses: - #3635886434 at tests/e2e/test_driver.py:1159 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@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.
Summary
Automated fix for #746 — Session is not set in object if connection fails, which raises 'AttributeError' on del.
Added a
hasattr(self, "session")guard at the top ofConnection.__del__in src/databricks/sql/client.py so that when the Session constructor raises beforeself.sessionis assigned, garbage-collecting the half-constructed Connection no longer triggersAttributeErrorvia theopenproperty. Verified with the E2E test that forces a constructor-stage failure (username/password → ValueError) and asserts nothing escapes del.Root cause & plan
Root cause: In Connection.init (src/databricks/sql/client.py:321),
self.session = Session(...)runs inside a try/except. If the Session CONSTRUCTOR raises before the assignment completes (e.g. client-side auth validation such as passing username/password -> ValueError, or use_kernel+use_sea -> ValueError, or an OAuth provider whose constructor makes a failing network call), thenself.sessionis never bound and the exception propagates out of init. When the half-constructed Connection is garbage-collected, del (client.py:464) executesif self.open:, and theopenproperty (client.py:509) evaluatesself.session.is_open. Sincesessionis absent, this raisesAttributeError: 'Connection' object has no attribute 'session', which Python reports as an 'Exception ignored in del' warning on stderr. Note: a bad host / bad token does NOT trigger this, because those fail in Session.open() (after self.session is assigned); only a constructor-stage failure leaves session unset.Files:
src/databricks/sql/client.py,tests/e2e/test_driver.pyPlanned coverage:
self.sessionis assigned must not raise AttributeError from del during garbage collection. Force a constructor-stage connection failure (against the live-warehouse connection params), let the failed Connection be collected, and assert that (a) the original connection error propagates and (b) no 'AttributeError: ... has no attribute session' / 'Exception ignored in del' is emitted. Will be red before the fix, green after. (del AttributeError when session attribute is never set due to a constructor-stage connection/initialization failure)Files changed
tests/e2e/test_driver.pysrc/databricks/sql/client.pyTest plan
tests/e2e/test_driver.py::TestPySQLConnectionFailureSuite::test_del_does_not_raise_when_session_never_set— fails (red) against the original code, passes (green) after the fix🤖 Generated by engineer-bot (bug-fix flow) — review before merge.