-
-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
gh-113280: Always close socket if SSLSocket creation failed #114659
gh-113280: Always close socket if SSLSocket creation failed #114659
Conversation
serhiy-storchaka
commented
Jan 27, 2024
•
edited by bedevere-app
bot
edited by bedevere-app
bot
- Issue: ResourceWarning in ssl.SSLSocket connected detection #113280
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Lib/ssl.py
Outdated
self = cls.__new__(cls, **kwargs) | ||
super(SSLSocket, self).__init__(**kwargs) | ||
sock_timeout = sock.gettimeout() | ||
sock.detach() | ||
try: | ||
sock_timeout = sock.gettimeout() | ||
sock.detach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to call sock.gettimeout()
and sock.detach()
outside the try/catch - as if either of those raise you could get a double close
sock_timeout = sock.gettimeout()
kwargs = dict(
family=sock.family, type=sock.type, proto=sock.proto,
fileno=sock.detatch()
)
self = cls.__new__(cls, **kwargs)
super(SSLSocket, self).__init__(**kwargs)
try:
self._context = context
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what if __new__
or __init__
raise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then they should be responsible for closing the passed in fileno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not. If you pass a fileno, you are responsible (in this case it is the responsibility of sock
until detach()
is called).
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
I added tests, although the original case reported in the issue is not covered by tests. But I added tests for two cases in which an explicit |
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
…thonGH-114659) (cherry picked from commit 0ea3662) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Thomas Grainger <tagrain@gmail.com>
GH-114995 is a backport of this pull request to the 3.12 branch. |
…thonGH-114659) (cherry picked from commit 0ea3662) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Thomas Grainger <tagrain@gmail.com>
GH-114996 is a backport of this pull request to the 3.11 branch. |
…thonGH-114659) Co-authored-by: Thomas Grainger <tagrain@gmail.com>
…thonGH-114659) Co-authored-by: Thomas Grainger <tagrain@gmail.com>