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

Jump to bottom

Inconsistent behavior of asyncio.Transport.get_extra_info("peername") on TLS vs. non-TLS connections #91635

Closed
aaugustin opened this issue Apr 17, 2022 · 6 comments · Fixed by #98563
Closed

Inconsistent behavior of asyncio.Transport.get_extra_info("peername") on TLS vs. non-TLS connections #91635

aaugustin opened this issue Apr 17, 2022 · 6 comments · Fixed by #98563
Labels
expert-asyncio pending The issue will be closed if no feedback is provided type-bug An unexpected behavior, bug, or error

Comments

@aaugustin
Copy link

aaugustin commented Apr 17, 2022

Bug report

After an asyncio.Transport is closed, transport.get_extra_info("peername") still returns the remote address if it's a non-TLS transport, while it doesn't return anything when it's a TLS transport. This inconsistency is surprising.

By TLS transport, I mean <class 'asyncio.sslproto._SSLProtocolTransport'> and by non-TLS transport, I mean <class 'asyncio.selector_events._SelectorSocketTransport'>. I observed this behavior on macOS.

Setup

>>> import asyncio
>>> loop = asyncio.get_event_loop()
<stdin>:1: DeprecationWarning: There is no current event loop

Test with TLS

>>> transport, protocol = loop.run_until_complete(loop.create_connection(asyncio.Protocol, "google.com", 443, ssl=True))
>>> transport
<asyncio.sslproto._SSLProtocolTransport object at 0x101c2c100>
>>> transport.get_extra_info("peername")
('142.250.74.238', 443)
>>> transport.close()
>>> loop.run_until_complete(asyncio.sleep(0.1))
>>> transport
<asyncio.sslproto._SSLProtocolTransport object at 0x101c2c100>
>>> transport.get_extra_info("peername")
>>>

Note that the second transport.get_extra_info("peername") doesn't return anything.

Test without TLS

>>> transport, protocol = loop.run_until_complete(loop.create_connection(asyncio.Protocol, "google.com", 80))
>>> transport
<_SelectorSocketTransport fd=8 read=polling write=<idle, bufsize=0>>
>>> transport.get_extra_info("peername")
('142.250.74.238', 80)
>>> transport.close()
>>> loop.run_until_complete(asyncio.sleep(0.1))
>>> transport
<_SelectorSocketTransport closed fd=8>
>>> transport.get_extra_info("peername")
('142.250.74.238', 80)
>>>

Note that the second transport.get_extra_info("peername") still returns the address.

Your environment

Python 3.10.0 (default, Nov 20 2021, 14:34:51) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
@aaugustin aaugustin added the type-bug An unexpected behavior, bug, or error label Apr 17, 2022
@aaugustin aaugustin mentioned this issue Apr 17, 2022
Closed
@kumaraditya303 kumaraditya303 self-assigned this Apr 17, 2022
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Apr 17, 2022

Tested on 3.11:

TLS transport does not return peername after closing as it sets it's underlying transport to None:

cpython/Lib/asyncio/sslproto.py

Lines 484 to 490 in 3289209

def _get_extra_info(self, name, default=None):
if name in self._extra:
return self._extra[name]
elif self._transport is not None:
return self._transport.get_extra_info(name, default)
else:
return default

However non-TLS transport does not wrap another transport so it continues to return in this case peername.

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Oct 22, 2022 •
edited

I am not sure if this a bug. After closing the transport, I don't think you can expect to get connection information, otherwise this has other unwanted consequences as the ssl context is destroyed so you cannot ssl ciphers etc. I would say you get all the info once connection is made and don't rely on this implementation detail.

@kumaraditya303 kumaraditya303 removed their assignment Oct 22, 2022
@python python deleted a comment from armw4 Oct 22, 2022
@gvanrossum gvanrossum added the pending The issue will be closed if no feedback is provided label Oct 22, 2022
@gvanrossum
Copy link
Member

gvanrossum commented Oct 22, 2022

Would like to hear from @aaugustin about the use case before we close this.

@aaugustin
Copy link
Author

aaugustin commented Oct 22, 2022 •
edited

Hello, you can find the original use case in the aforementioned issue: aaugustin/websockets#1136

So, if an application wants this info after the connection is closed, it has to be cached at one of the following three levels:

A. standard library (Python)
B. third-party library (websockets or any other asyncio-based library)
C. application

Currently, the answer is "A. for plain TCP connections, C. for TCP + TLS connections". This behavior appears to be an implementation detail rather than an intentional behavior. So we're in Hyrum's law territory....

It could be better to raise an exception when get_extra_info is called on a closed transport. But it's hard to justify the backwards incompatibility at this point: users may depend on the fact that this currently works for plain TCP connections.

In the end, I don't care which choice we make: websockets will promise exactly what Python promises. It will be zero effort for me in all cases :-)

@bedevere-bot bedevere-bot mentioned this issue Oct 23, 2022
Merged
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Oct 23, 2022

PR #98563 adds clarification that transports should not be used after they are closed. IMO a doc update is enough for this.

gvanrossum pushed a commit that referenced this issue Oct 24, 2022
@kumaraditya303
GH-91635: clarify docs about closing of transport in asyncio (#98563)
2fdcc6f
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 24, 2022
@kumaraditya303 @miss-islington
pythonGH-91635: clarify docs about closing of transport in asyncio (p…
d885370 
…ythonGH-98563)

(cherry picked from commit 2fdcc6f)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot bedevere-bot mentioned this issue Oct 24, 2022
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 24, 2022
@kumaraditya303 @miss-islington
pythonGH-91635: clarify docs about closing of transport in asyncio (p…
400d769 
…ythonGH-98563)

(cherry picked from commit 2fdcc6f)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot bedevere-bot mentioned this issue Oct 24, 2022
Merged
miss-islington added a commit that referenced this issue Oct 24, 2022
@miss-islington @kumaraditya303
GH-91635: clarify docs about closing of transport in asyncio (GH-98563)
032d127 
(cherry picked from commit 2fdcc6f)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
miss-islington added a commit that referenced this issue Oct 24, 2022
@miss-islington @kumaraditya303
GH-91635: clarify docs about closing of transport in asyncio (GH-98563)
39e914c 
(cherry picked from commit 2fdcc6f)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@gvanrossum gvanrossum closed this as not planned Won't fix, can't repro, duplicate, stale Oct 24, 2022
@gvanrossum
Copy link
Member

gvanrossum commented Oct 24, 2022

Marking as not planned since we decided to keep the existing inconsistent behavior, and just document that you shouldn't do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
expert-asyncio pending The issue will be closed if no feedback is provided type-bug An unexpected behavior, bug, or error
Projects
asyncio
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

GH-91635: clarify docs about closing of transport in asyncio kumaraditya303/cpython
4 participants
@aaugustin @gvanrossum @kumaraditya303 @AlexWaygood