Skip to content
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

Sync Connection garbage collection closing #1601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thewhaleking
Copy link

Ensures the sync connection is closed when the object is garbage collected.

While in a perfect world, everyone would correctly use the context manager, I've found this is not the case. In a situation like this, it's possible for a memory leak to be caused by the connection never being closed. This ensures graceful closing when the Connection object is garbage collected.

@aaugustin
Copy link
Member

I agree that it's worth looking into the best behavior there. However, I'm unsure what that behavior is.

As a reminder:

Due to the precarious circumstances under which del() methods are invoked, exceptions that occur during their execution are ignored, and a warning is printed to sys.stderr instead.

I believe that close() is designed to not raise exceptions in general but I'm not fully confident that it never raises exceptions. Also, doing a clean cloure withclose(1001) (IMO the code should be 1001 in that case) can take up to close_timeout, joins a thread, etc. I don't feel comfortable doing that in __del__.

I'm leaning towards doing an unclean closure, either with fail() or simply by closing the socket. What do you think?

For completeness, we must do the same in the asyncio implementation. I don't think we can do it usefully in the Sans-I/O implementation, to be confirmed. And we need to add a test.

@thewhaleking
Copy link
Author

I'm leaning towards doing an unclean closure, either with fail() or simply by closing the socket. What do you think?

For completeness, we must do the same in the asyncio implementation. I don't think we can do it usefully in the Sans-I/O implementation, to be confirmed. And we need to add a test.

I agree with the fail() use in this case. I created this PR more of as a way to ask your thoughts on the best way to handle this, as I tend to get better results than opening Issues (my last one of aiohttp took two years to resolve).

Doing this in the asyncio implementation will be a little tricky, as we can't await, but I'll dig around a bit when I get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants