Closed Snaacky closed 2 years ago
Thanks for submitting this PR! The problem I have with this is that it doubles/overloads the semantics of context managers on db
: they've been doing transaction management since the first release of dataset, not connection management. Connection management may have been a better use for it, but at this point it would be a breaking change - a surprising escalation of what a context manager does. This probably does not matter in applications that use only one big wrapping tx, but in scripts that use smaller transaction scopes it would cause unexpected issues.
Perhaps what we could be doing is a different method, maybe dataset.connect_with
, that allows wrapping connection context... WDYT?
Perhaps what we could be doing is a different method, maybe dataset.connect_with, that allows wrapping connection context... WDYT?
Would connect_with()
more or less be a wrapper for connect()
with a different exit routine then?
Yep, exactly.
Closing this for now as it's breaking backwards compatibility. I think the wrapper path (maybe with a better name than connect_with
) would be best.
Currently, when a database connection made with a context manager exits, the connection to the database isn't actually closed.
For example, the following code will cause a stock MySQL database to lock up in about 15-20 seconds when it hits 151 simultaneous open connections: