fix(python/adbc_driver_manager): close Cursors when closing Connection #3810
Conversation
… Python dbapi connection Closes apache#3713
prmoore77
force-pushed
the
fix-auto-close-dbapi-cursors
branch
from
be5af22 to
93790ff
Compare
lidavidm
changed the title
amoeba
left a comment
amoeba
left a comment
There was a problem hiding this comment.
This looks reasonable to me. I also manually tested flightsql, mssql, and postgresql and the tests you added here run correctly for those drivers as well.
One comment about a docstring.
Co-authored-by: Bryce Mecum <[email protected]>
|
Thanks, @amoeba ! |
|
PS I'll defer to @lidavidm's review before merging. |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
I think we should file a separate issue for this - it's fine to have Python guard this but I think the current issue is more about having drivers themselves handle this.
Is it ok to merge this in the meantime - for Python users to benefit? |
|
Please make a separate issue to attach to this PR so I can merge. |
|
Ah, the issue sounds like Python should handle it...my original comment was about having the drivers handle it so that's not what I expected. Ok then. Let me just file the new issue for the drivers so that we don't lose the context. |
|
|
||
| # Close all open cursors first to avoid RuntimeError from | ||
| # AdbcConnection about open children | ||
| for cursor in list(self._cursors): |
There was a problem hiding this comment.
nit: can't you iterate the WeakSet directly?
It appears the list guard is well out of date: https://stackoverflow.com/questions/12428026/safely-iterating-over-weakkeydictionary-and-weakvaluedictionary
There was a problem hiding this comment.
This should be fixed now.
| # Close all open cursors first to avoid RuntimeError from | ||
| # AdbcConnection about open children | ||
| for cursor in list(self._cursors): | ||
| cursor.close() |
There was a problem hiding this comment.
If close raises, the other references won't get closed. Can we handle that? (We should catch Exception and either prepare to rethrow it, or add it as a suppressed exception.)
For testing purposes, we could factor it out into a small internal helper that can then be directly tested with non-Cursor objects containing a close method.
There was a problem hiding this comment.
I'm not sure what to do here - b/c if we fail to close one (or more) of the connection's cursors for some reason - won't we hit the original error?
RuntimeError: Cannot close AdbcConnection with open AdbcStatement
There was a problem hiding this comment.
Sure, but we should at least try and clean up all of the ones we can, IMO
There was a problem hiding this comment.
Ok - I've pushed up a change suppressing cursor close errors. The original error should still bubble up in case we leave one opened.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
If we're going to do this, IMO it should at least get emitted as a ResourceWarning or something.
There was a problem hiding this comment.
I've modified to emit a ResourceWarning.
|
The formatter is unhappy :) |
This should be fixed now. Thanks for your help/review. |
|
I've made another formatting fix |
Updated ADBC Flight SQL Client driver version. Fixed cursor leak bug. Removed ADBC Connection proxy - b/c the cursor close bug has been fixed - apache/arrow-adbc#3810
3 participants
Closes #3713