Enhance Cursor close handling and context manager exception management to prevent server side resource leaks #554
Conversation
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
madhav-db
requested review from
deeksha-db,
samikshya-db,
jprakash-db,
yunbodeng-db,
jackyhu-db and
benc-db
as code owners
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
madhav-db
changed the title
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
gopalldb
reviewed
May 14, 2025
gopalldb
approved these changes
May 14, 2025
jprakash-db
reviewed
May 15, 2025
jprakash-db
reviewed
May 15, 2025
…nsure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
jprakash-db
approved these changes
May 21, 2025
varun-edachali-dbx
pushed a commit
to varun-edachali-dbx/databricks-sql-python-dev
that referenced
this pull request
May 23, 2025
…t to prevent server side resource leaks (databricks#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
to varun-edachali-dbx/databricks-sql-python-dev
that referenced
this pull request
May 24, 2025
…t to prevent server side resource leaks (databricks#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
saishreeeee
pushed a commit
that referenced
this pull request
May 26, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add
saishreeeee
pushed a commit
that referenced
this pull request
May 26, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: Sai Shree Pradhan <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 26, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 26, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 27, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 28, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 28, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
pushed a commit
that referenced
this pull request
May 28, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]>
varun-edachali-dbx
added a commit
that referenced
this pull request
May 28, 2025
* decouple session class from existing Connection ensure maintenance of current APIs of Connection while delegating responsibility Signed-off-by: varun-edachali-dbx <[email protected]> * add open property to Connection to ensure maintenance of existing API Signed-off-by: varun-edachali-dbx <[email protected]> * update unit tests to address ThriftBackend through session instead of through Connection Signed-off-by: varun-edachali-dbx <[email protected]> * chore: move session specific tests from test_client to test_session Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) as in CONTRIBUTING.md Signed-off-by: varun-edachali-dbx <[email protected]> * use connection open property instead of long chain through session Signed-off-by: varun-edachali-dbx <[email protected]> * trigger integration workflow Signed-off-by: varun-edachali-dbx <[email protected]> * fix: ensure open attribute of Connection never fails in case the openSession takes long, the initialisation of the session will not complete immediately. This could make the session attribute inaccessible. If the Connection is deleted in this time, the open() check will throw because the session attribute does not exist. Thus, we default to the Connection being closed in this case. This was not an issue before because open was a direct attribute of the Connection class. Caught in the integration tests. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: de-complicate earlier connection open logic earlier, one of the integration tests was failing because 'session was not an attribute of Connection'. This is likely tied to a local configuration issue related to unittest that was causing an error in the test suite itself. The tests are now passing without checking for the session attribute. c676f9b Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "fix: de-complicate earlier connection open logic" This reverts commit d6b1b19. Signed-off-by: varun-edachali-dbx <[email protected]> * [empty commit] attempt to trigger ci e2e workflow Signed-off-by: varun-edachali-dbx <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: varun-edachali-dbx <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit dbb2ec5, reversing changes made to 7192f11. Signed-off-by: varun-edachali-dbx <[email protected]> * Reapply "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit bdb8381. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: separate session opening logic from instantiation ensures correctness of self.session.open call in Connection Signed-off-by: varun-edachali-dbx <[email protected]> * fix: use is_open attribute to denote session availability Signed-off-by: varun-edachali-dbx <[email protected]> * fix: access thrift backend through session Signed-off-by: varun-edachali-dbx <[email protected]> * chore: use get_handle() instead of private session attribute in client Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * fix: remove accidentally removed assertions Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]> Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]> Co-authored-by: Sai Shree Pradhan <[email protected]>
saishreeeee
added a commit
that referenced
this pull request
May 30, 2025
* PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * removed TelemetryHelper Signed-off-by: Sai Shree Pradhan <[email protected]> * [PECOBLR-361] convert column table to arrow if arrow present (#551) Signed-off-by: Sai Shree Pradhan <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: Sai Shree Pradhan <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> * Update github actions run conditions (#569) More conditions to run github actions Signed-off-by: Sai Shree Pradhan <[email protected]> * Added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed example Signed-off-by: Sai Shree Pradhan <[email protected]> * changed to doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * removed self.telemetry close line Signed-off-by: Sai Shree Pradhan <[email protected]> * grouped classes Signed-off-by: Sai Shree Pradhan <[email protected]> * formatting Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * added more descriptive comments, put dataclasses in a sub-folder Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed default attributes ordering Signed-off-by: Sai Shree Pradhan <[email protected]> * changed file names Signed-off-by: Sai Shree Pradhan <[email protected]> * added enums to models folder Signed-off-by: Sai Shree Pradhan <[email protected]> * removed telemetry batch size Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Shivam Raj <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]>
saishreeeee
pushed a commit
that referenced
this pull request
Jun 4, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add
saishreeeee
added a commit
that referenced
this pull request
Jun 4, 2025
* PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * removed TelemetryHelper Signed-off-by: Sai Shree Pradhan <[email protected]> * [PECOBLR-361] convert column table to arrow if arrow present (#551) Signed-off-by: Sai Shree Pradhan <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: Sai Shree Pradhan <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> * Update github actions run conditions (#569) More conditions to run github actions Signed-off-by: Sai Shree Pradhan <[email protected]> * Added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed example Signed-off-by: Sai Shree Pradhan <[email protected]> * changed to doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * removed self.telemetry close line Signed-off-by: Sai Shree Pradhan <[email protected]> * grouped classes Signed-off-by: Sai Shree Pradhan <[email protected]> * formatting Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * added more descriptive comments, put dataclasses in a sub-folder Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed default attributes ordering Signed-off-by: Sai Shree Pradhan <[email protected]> * changed file names Signed-off-by: Sai Shree Pradhan <[email protected]> * added enums to models folder Signed-off-by: Sai Shree Pradhan <[email protected]> * removed telemetry batch size Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Shivam Raj <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]>
saishreeeee
pushed a commit
that referenced
this pull request
Jun 4, 2025
…t to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: Sai Shree Pradhan <[email protected]>
saishreeeee
added a commit
that referenced
this pull request
Jun 4, 2025
* PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * removed TelemetryHelper Signed-off-by: Sai Shree Pradhan <[email protected]> * [PECOBLR-361] convert column table to arrow if arrow present (#551) Signed-off-by: Sai Shree Pradhan <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: Sai Shree Pradhan <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> * Update github actions run conditions (#569) More conditions to run github actions Signed-off-by: Sai Shree Pradhan <[email protected]> * Added classes required for telemetry Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed example Signed-off-by: Sai Shree Pradhan <[email protected]> * changed to doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * removed self.telemetry close line Signed-off-by: Sai Shree Pradhan <[email protected]> * grouped classes Signed-off-by: Sai Shree Pradhan <[email protected]> * formatting Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed doc string Signed-off-by: Sai Shree Pradhan <[email protected]> * added more descriptive comments, put dataclasses in a sub-folder Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed default attributes ordering Signed-off-by: Sai Shree Pradhan <[email protected]> * changed file names Signed-off-by: Sai Shree Pradhan <[email protected]> * added enums to models folder Signed-off-by: Sai Shree Pradhan <[email protected]> * removed telemetry batch size Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Shivam Raj <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]> Signed-off-by: Sai Shree Pradhan <[email protected]>
varun-edachali-dbx
added a commit
that referenced
this pull request
Jun 18, 2025
* decouple session class from existing Connection ensure maintenance of current APIs of Connection while delegating responsibility Signed-off-by: varun-edachali-dbx <[email protected]> * add open property to Connection to ensure maintenance of existing API Signed-off-by: varun-edachali-dbx <[email protected]> * update unit tests to address ThriftBackend through session instead of through Connection Signed-off-by: varun-edachali-dbx <[email protected]> * chore: move session specific tests from test_client to test_session Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) as in CONTRIBUTING.md Signed-off-by: varun-edachali-dbx <[email protected]> * use connection open property instead of long chain through session Signed-off-by: varun-edachali-dbx <[email protected]> * trigger integration workflow Signed-off-by: varun-edachali-dbx <[email protected]> * fix: ensure open attribute of Connection never fails in case the openSession takes long, the initialisation of the session will not complete immediately. This could make the session attribute inaccessible. If the Connection is deleted in this time, the open() check will throw because the session attribute does not exist. Thus, we default to the Connection being closed in this case. This was not an issue before because open was a direct attribute of the Connection class. Caught in the integration tests. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: de-complicate earlier connection open logic earlier, one of the integration tests was failing because 'session was not an attribute of Connection'. This is likely tied to a local configuration issue related to unittest that was causing an error in the test suite itself. The tests are now passing without checking for the session attribute. c676f9b Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "fix: de-complicate earlier connection open logic" This reverts commit d6b1b19. Signed-off-by: varun-edachali-dbx <[email protected]> * [empty commit] attempt to trigger ci e2e workflow Signed-off-by: varun-edachali-dbx <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: varun-edachali-dbx <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit dbb2ec5, reversing changes made to 7192f11. Signed-off-by: varun-edachali-dbx <[email protected]> * Reapply "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit bdb8381. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: separate session opening logic from instantiation ensures correctness of self.session.open call in Connection Signed-off-by: varun-edachali-dbx <[email protected]> * fix: use is_open attribute to denote session availability Signed-off-by: varun-edachali-dbx <[email protected]> * fix: access thrift backend through session Signed-off-by: varun-edachali-dbx <[email protected]> * chore: use get_handle() instead of private session attribute in client Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * fix: remove accidentally removed assertions Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]> Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]> Co-authored-by: Sai Shree Pradhan <[email protected]>
varun-edachali-dbx
added a commit
that referenced
this pull request
Jun 18, 2025
* decouple session class from existing Connection ensure maintenance of current APIs of Connection while delegating responsibility Signed-off-by: varun-edachali-dbx <[email protected]> * add open property to Connection to ensure maintenance of existing API Signed-off-by: varun-edachali-dbx <[email protected]> * update unit tests to address ThriftBackend through session instead of through Connection Signed-off-by: varun-edachali-dbx <[email protected]> * chore: move session specific tests from test_client to test_session Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) as in CONTRIBUTING.md Signed-off-by: varun-edachali-dbx <[email protected]> * use connection open property instead of long chain through session Signed-off-by: varun-edachali-dbx <[email protected]> * trigger integration workflow Signed-off-by: varun-edachali-dbx <[email protected]> * fix: ensure open attribute of Connection never fails in case the openSession takes long, the initialisation of the session will not complete immediately. This could make the session attribute inaccessible. If the Connection is deleted in this time, the open() check will throw because the session attribute does not exist. Thus, we default to the Connection being closed in this case. This was not an issue before because open was a direct attribute of the Connection class. Caught in the integration tests. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: de-complicate earlier connection open logic earlier, one of the integration tests was failing because 'session was not an attribute of Connection'. This is likely tied to a local configuration issue related to unittest that was causing an error in the test suite itself. The tests are now passing without checking for the session attribute. c676f9b Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "fix: de-complicate earlier connection open logic" This reverts commit d6b1b19. Signed-off-by: varun-edachali-dbx <[email protected]> * [empty commit] attempt to trigger ci e2e workflow Signed-off-by: varun-edachali-dbx <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: varun-edachali-dbx <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit dbb2ec5, reversing changes made to 7192f11. Signed-off-by: varun-edachali-dbx <[email protected]> * Reapply "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit bdb8381. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: separate session opening logic from instantiation ensures correctness of self.session.open call in Connection Signed-off-by: varun-edachali-dbx <[email protected]> * fix: use is_open attribute to denote session availability Signed-off-by: varun-edachali-dbx <[email protected]> * fix: access thrift backend through session Signed-off-by: varun-edachali-dbx <[email protected]> * chore: use get_handle() instead of private session attribute in client Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * fix: remove accidentally removed assertions Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]> Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]> Co-authored-by: Sai Shree Pradhan <[email protected]>
8 tasks
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.
What type of PR is this?
Description
This PR attempts to fix an issue where database connections were not properly closing, resulting in "lame duck" sessions that kept SQL warehouses active unnecessarily.
When clients disconnected, operation handles were not properly being closed on the server side, potentially leading to resource leaks. The
Cursor.close()methodwas simply nullifying the operation handle reference without properly closing it on the thrift backend.Besides this, in
Cursor.exit(), added better error handling to prevent exceptions during cursor closure from suppressing original exceptions, which is particularly important for user introduced interruptions (eg. Keyboard interrupt). Before this fix, when users interrupted long-running queries, the exception from cursor.close() could mask the KeyboardInterrupt, preventing the application from properly responding to cancellation requests and potentially leaving resources uncleaned.How is this tested?
Added new unit tests to verify behaviour
Related Tickets & Documents