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

Ignore socket failure to close during del #379

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Conversation

kevinkjt2000
Copy link
Contributor

Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, although there is one detail I'd like to mention:

The commit here seems to have been made under a different git account ([email protected], instead of the annonymous github assigned email you usually commit under: [email protected]). I assume you accidentally used your work email here, which has then also lead to the commit getting an "Unverified" status on github, since this git email doesn't seem to be associated with a registered github user.

This isn't at all an issue that would prevent merging, however you might want to force-push and fix this. If you're fine with there being an unverified commit from this git account in the repo though, you can simply merge it, as functionality-wise it looks good.

@ItsDrike ItsDrike added type: bug Something isn't working area: API Related to core API of the project state: approved The issue has received an approval from the maintainers labels Aug 22, 2022
Copy link
Member

@PerchunPak PerchunPak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this try-ecept to close method instead of __del__?

@ItsDrike
Copy link
Member

ItsDrike commented Aug 22, 2022

Maybe move this try-ecept to close method instead of __del__?

I thought of that too, but I'm a believer of not silently suppressing errors, if the user called close explicitly, they should see it if the socket was closed already and if they're interested in suppressing that failure, they can handle it themselves in their try-except. This does make sense in __del__ as the user may have explicitly called close, so we don't want to cause weird errors that occur on variable going out of scope and getting garbage collected, but I don't think it would actually make much sense to do so in close directly.

@PerchunPak
Copy link
Member

Better would be at least document this error in close method.

@kevinkjt2000
Copy link
Contributor Author

kevinkjt2000 commented Aug 22, 2022

Better would be at least document this error in close method.

I suspect that these errors won't happen under normal circumstances. Closing a socket ought to work. It is only during garbage collection calling __del__ where behavior is exceptional and we know that it is safe to ignore.

@kevinkjt2000 kevinkjt2000 merged commit f50da2b into master Aug 22, 2022
@kevinkjt2000 kevinkjt2000 deleted the sock-close branch August 22, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Related to core API of the project state: approved The issue has received an approval from the maintainers type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants