Repository navigation
Add loop.connect_accepted_socket #71579
Description
| BPO | 27392 |
|---|---|
| Nosy | @gvanrossum, @vstinner, @jimfulton, @1st1 |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/1st1'
closed_at = <Date 2016-10-06.18:10:40.888>
created_at = <Date 2016-06-26.16:05:16.970>
labels = ['type-feature', 'expert-asyncio']
title = 'Add loop.connect_accepted_socket'
updated_at = <Date 2016-11-07.20:36:55.274>
user = 'https://github.com/jimfulton'bugs.python.org fields:
activity = <Date 2016-11-07.20:36:55.274>
actor = 'yselivanov'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2016-10-06.18:10:40.888>
closer = 'yselivanov'
components = ['asyncio']
creation = <Date 2016-06-26.16:05:16.970>
creator = 'j1m'
dependencies = []
files = ['43718']
hgrepos = []
issue_num = 27392
keywords = ['patch']
message_count = 44.0
messages = ['269293', '269297', '269315', '269492', '269493', '269494', '269513', '269515', '269516', '269519', '269524', '269536', '269540', '269545', '269546', '269547', '270046', '270052', '270059', '270060', '270061', '270062', '270133', '270182', '270185', '270272', '270273', '270411', '270785', '270850', '272129', '272177', '272178', '272179', '272203', '272204', '272205', '272206', '272207', '272232', '272233', '272280', '278201', '280236']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'vstinner', 'j1m', 'python-dev', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27392'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']
Activity
jimfulton commentedon Jun 26, 2016
The event loop create_connection method can take a socket to create a connection on an existing socket, including sockets obtained via an external listener. If an SSL context is provided, however, it assumes it's creating a client connection, making it impossible to use it in a server.
I've recently ported ZEO to asyncio. My original implementation used a separate thread for each connection and a thread for listening for connections. I think there are cases where this makes a lot of sense. Blocky operations only affect one client and, IMO, using an asynchronous model can simplify networking code even when there's only one connection. Unfortunately, this didn't work on Linux with SSL due to issues with SSL and non-blocking sockets. (Oddly, it worked fine on Mac OS X.)
I've switched my code to use create_server, but this has led to stability problems. Beyond http://bugs.python.org/issue27386, I'm seeing a lot of test instability. I can't get through the ZEO tests without some test failing, although the tests pass when run individually. I suspect that this is due to a problem in my error handling, asyncio's error handling, or likely both.
Note that the ZEO test suite is pretty ruthless, doing whatever they can to break ZEO servers and clients.
I have a version of my multi-threaded code that monkey-patches loop instances to pass server_side=True to _make_ssl_transport. With that awful hack, I can use an external listener and tests usually run without errors. :)
I'd be more than happy to create a PR that adds this option (including test and docs). Please just give me the word. :)
1st1 commentedon Jun 26, 2016
BTW, did you try to run ZEO tests on uvloop? I'm just curious if stability is somehow related to asyncio design, or its just implementation quirks.
gvanrossum commentedon Jun 26, 2016
I'm confused. create_connection() is meant for creating client connection,
so I don't think a server_side flag makes sense. (There are lower-level
internal APIs that do take a server_side flag, but create_connection() is
just one caller for these.)
If create_server() is buggy we should fix those bugs!
jimfulton commentedon Jun 29, 2016
Tests are also unstable with uvloop. (Although uvloop doesn't have http://bugs.python.org/issue27386 at least.)
I was eventually able to wrestle the test into submission using asyncio.Server. I suspect that some of this had to do with issues closing connections at the end of tests. I made my close logic more paranoid and that *seemed* to help. Some of the instability was due to test bugs that were activated by the different timing characteristics of using asyncio.Server.
jimfulton commentedon Jun 29, 2016
Guido, IMO, there's value in having the ability to accept connections independently of handling them.
It wasn't clear to me from reading the documentation that create_connection is only meant for making client connections, especially given that it can take an already connected socket. It works well for non-SSL server sockets and would work for SSL with a trivial change.
This issue was discussed on the tulip list or issue tracker a couple of years ago. (Sorry, I can't find the link ATM.) I think it was dismissed because no one felt like looking into it and because you asserted that it makes no sense to use an async library when handling each connection in a separate thread.
IMO, async loop is useful even for a single connection when reading and writing are not purely synchronous. In ZEO it's very useful that that the server can send data to a client independent of requests the client is making. (Technically, under the hood this often means that there are really 2 I/O channels, the client's TCP connection and whatever mechanism the loop uses to allow itself to be woken asynchronously as in call_soon_threadsafe.)
jimfulton commentedon Jun 29, 2016
I agree that if create_server (or asyncio.Server) is buggy, it should be fixed!
However, IMO, it's useful to be able to accept a connection outside of an asyncio event loop and then hand the loop the connected socket.
1st1 commentedon Jun 29, 2016
Looks like what you're asking for is a way to wrap existing socket object into a (transport, protocol) pair. I'm -1 to add this new semantics to loop.create_connection, as I think it will complicate it too much.
However, we can consider adding something like
loop.wrap_socket(protocol_factory, sock) -> Transport
jimfulton commentedon Jun 29, 2016
Yury, I'm curious what you think the socket argument to create_connection is about.
jimfulton commentedon Jun 29, 2016
BTW, a problem with this proposal that I realized after submitting it is that it changes an API that has multiple implementations, including implementations outside of the Python codebase. Arguably, this would require a PEP, at which point the change is no-longer trivial. :)
1st1 commentedon Jun 29, 2016
:) The current intended purpose of create_connection is to create a client connection. You're proposing to add a new argument -- server_side -- which I think will confuse the users of create_connection.
What I'm saying is that we may consider creating a low-level loop.wrap_socket, which would be generic and suitable to be used for both client and server connections. We could even refactor create_connection to use wrap_socket when 'sock' argument is passed to it.
We already have something similar, although it's a private API -- _make_socket_transport.
No need for a PEP; Guido's approval is enough usually.
jimfulton commentedon Jun 29, 2016
Perhaps. I'll note that the word "client" appears nowhere in the documentation of create_connection. I needed a way to wrap a socket and create_connection took one. Wrapping a server socket seemed to be to be the most likely use case for it. <shrug>
Right. That's what I'm monkey-patching now to work around this, mostly as an experiment.
/me holds breath
gvanrossum commentedon Jun 29, 2016
Hm. The docs in PEP-3156 do mention that create_connection() is for clients (though it weakens this with "typically"): https://www.python.org/dev/peps/pep-3156/#internet-connections
I always think of TCP connections (which is what create_connection() is about) as essentially asymmetrical -- the server uses bind() and listen() and then sits in an accept() loop accepting connections, while the client reaches out to the server using connect(). And create_connection() is a wrapper around that connect() call; for servers we have create_server(). (The asymmetry in naming reflects the asymmetry in functionality and API.)
You can pass a pre-connected socket in to create_connection() for edge cases where you need control over how the socket is created and initialized or when there's a 3rd party library that can give you a connected socket that you want/need to use. Similarly, you can pass a pre-bound socket to create_server().
I guess at the lower, TCP level, there isn't much of a difference between a client-side socket and the kind of server-side socket that's returned by accept(). And maybe that's also the case for SSL (I've never actually understood most of the details of using SSL).
Maybe it's just culture shock? Or maybe we just need a public API that roughly represents the pair of calls to _make_ssl_transport() and _make_socket_transport() that are currently appearing both in _create_connection_transport() and in _accept_connection2(), plus some of the code around it that's a little tricky?
jimfulton commentedon Jun 29, 2016
With SSL, the protocol is a little different clients and servers, although that may just be in the handshake. I'm no SSL expert by any means. When you call wrap_socket on an SSLContext, you can pass server_side, which defaults to False. If you get this wrong, you end up with an SSL protocol error.
FWIW, here's my awful monkey patch to work around this experimentally in ZEO:
zopefoundation/ZEO@daca97c#diff-248404a51b1503a38c3319c85e6c1c5aR174
20 remaining items
jimfulton commentedon Aug 8, 2016
idk if the patch got merged.
I just added the last comment for informational purposes. :)
Perhaps this issue can be closed.
1st1 commentedon Aug 8, 2016
connect_accepted_socket was merged. The docs patch is still pending (nothing wrong with it, I just need to commit it)
1st1 commentedon Aug 9, 2016
Hm, I'm working on adding connect_accepted_socket to the uvloop. There is one difference between connect_accepted_socket and create_server/create_unix_server: the latter APIs forbid to pass boolean
sslargument, they requiresslto be an instance ofSSLContext.Should we have the same requirement for the 'ssl' argument of newly added 'connect_accepted_socket'?
1st1 commentedon Aug 9, 2016
Also I think we should add a check, that the passed socket is AF_UNIX or AF_INET(6)
gvanrossum commentedon Aug 9, 2016
Oh, I see. create_connection(..., ssl=True) creates a default SSLContext,
but create_server(..., ssl=True) is invalid, it requires
ssl=SSLContext(...). I like the latter for connect_accepted_socket(). I
think Jim will happily comply.
What would happen if some other socket type was passed? Would anything go
wrong, assuming it's a socket type that understands connections? (I think
checking for SOCK_STREAM is more important maybe).
1st1 commentedon Aug 9, 2016
In uvloop I have to create different libuv handles for AF_UNIX and AF_INET. I think I can only support those families. Cheking for SOCK_STREAM is also important.
gvanrossum commentedon Aug 9, 2016
I think it's okay if uvloop only handles those socket types. But if asyncio
may be able to handle other types we shouldn't reject them just because we
haven't heard about them. (E.g. IIRC someone was using Bluetooth sockets.)
jimfulton commentedon Aug 9, 2016
WRT boolean for SSL, I think it's very common for clients to verify server certificates, but relatively uncommon for servers to require client certificates. The impression I have from reading docs and stack overflow posts that the most common use case for the SSL module is connection to HTTPS sites. For this use case, using a default context makes a lot of sense.
It seems extremely unlikely to me for a server to use a default context.
But I'm not an SSL expert.
jimfulton commentedon Aug 9, 2016
+1 restricting uvloop to AF_INET or AF_UNIX and SOCK_STREAM, at least until someone requests something else.
1st1 commentedon Aug 9, 2016
I think in this case we should just mimic the API of create_server and create_unix_server.
As for restricting socket family type - I think Guido is right. Although I'd love uvloop to be 100% compatible, I guess we shouldn't add artificial restrictions: if asyncio already supports AF_BLUETOOTH then let's keep it that way.
1st1 commentedon Oct 6, 2016
AFAICT this issue was resolved in python/asyncio#378. Closing this one. Thanks, Jim!
python-dev commentedon Nov 7, 2016
New changeset 3e6570231c80 by Yury Selivanov in branch '3.5':
Issue bpo-27392: Document loop.connect_accepted_socket()
https://hg.python.org/cpython/rev/3e6570231c80
New changeset 6811df9e9797 by Yury Selivanov in branch '3.6':
Merge 3.5 (issue bpo-27392)
https://hg.python.org/cpython/rev/6811df9e9797
New changeset 573bc1f9900e by Yury Selivanov in branch 'default':
Merge 3.6 (issue bpo-27392)
https://hg.python.org/cpython/rev/573bc1f9900e
[-]Add a server_side keyword parameter to create_connection[/-][+]Add loop.connect_accepted_socket[/+]