-
Notifications
You must be signed in to change notification settings - Fork 408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Leaked Connections should close socket #323
Comments
Connection objects currently stay alive and open if they are not explicitly closed or terminated. GC won't happen to them because event loops have a strong reference to their underlying Transport object. By replacing a strong Connection<->Protocol reference with a weak one, we are able to implement Connection.__del__() method that: * issues a warning if a Connection object is being GCed prior to be explicitly closed; * terminates the underlying Protocol and Transport, effectively closing the open network connection to the Postgres server. When in asyncio debug mode (enabled by PYTHONASYNCIODEBUG env variable or explicitly with `loop.set_debug(True)`) Connection objects save the traceback of their origin and later use it to make the GC warning clarer. Addresses #323.
PR #324 partially addresses this by:
You need to allow the event loop to run for a little bit for the termination callbacks to fire. If you close the event loop immediately after calling |
Connection objects currently stay alive and open if they are not explicitly closed or terminated. GC won't happen to them because event loops have a strong reference to their underlying Transport object. By replacing a strong Connection<->Protocol reference with a weak one, we are able to implement Connection.__del__() method that: * issues a warning if a Connection object is being GCed prior to be explicitly closed; * terminates the underlying Protocol and Transport, effectively closing the open network connection to the Postgres server. When in asyncio debug mode (enabled by PYTHONASYNCIODEBUG env variable or explicitly with `loop.set_debug(True)`) Connection objects save the traceback of their origin and later use it to make the GC warning clarer. Addresses #323.
Awesome, thanks for the fix (and for the terminate explanation). This fixes the the leak in the context that I noticed it (but I'll let a maintainer close this issue, since the "PR #324 partially addresses this" makes it sound like there may have been more work left to do here?) |
I used "partially" because it's impossible to fully solve the problem in all cases. The loop holds strong references to its transports, so there might be some hypothetical situation where the only solution is to manually close the Connection object. I.e. if you forget to close an event loop in your long-living application, that zombie event loop will keep its connections forever open. It's highly unlikely that this happens in a properly engineered application though. :) |
This is fixed in #324 |
the issue with a local PostgreSQL install?: It's a docker postgres:latest
uvloop?: yes
In the face of timeouts/other exceptions, it may not always be possible to have a reference to a Connection (that was successfully initiated) and call
.close()
on it. So, leaked Connections should clean up after themselves. For example, runningasyncio.get_event_loop().run_until_complete(asyncpg.connect("postgres://postgres:postgres@localhost/postgres"))
seems to leave the socket open forever (at least on my platform).When attempting to address this, I noticed a couple of things that may or may not be related bugs:
The text was updated successfully, but these errors were encountered: