Add loop.connect_accepted_socket

[jimfulton]

BPO	27392
Nosy	@gvanrossum, @vstinner, @jimfulton, @1st1
Files	connect_accepted_socket-doc.patch

^{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']

[jimfulton]

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]

On Jun 26, 2016, at 12:05 PM, Jim Fulton <report@bugs.python.org> wrote:

I've switched my code to use create_server, but this has led to stability problems.

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]

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]

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]

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]

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]

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.

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]

Yury, I'm curious what you think the socket argument to create_connection is about.

[jimfulton]

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]

Yury, I'm curious what you think the socket argument to create_connection is about.

:) 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.

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. :)

No need for a PEP; Guido's approval is enough usually.

[jimfulton]

> Yury, I'm curious what you think the socket argument to create_connection is about.

:) 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.

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>

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.

Right. That's what I'm monkey-patching now to work around this, mostly as an experiment.

> 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. :)

No need for a PEP; Guido's approval is enough usually.

/me holds breath

[gvanrossum]

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]

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

[gvanrossum]

Rather tham monkey-patching, in general I recommend just copying some
code from the asyncio library and calling that. In this case you'd be
copying a tiny bit of code from create_connection(). You'd still be
calling an internal API, _make_ssl_transport(), but your code would
still be less likely to change when some part of the asyncio library
changes than with monkey-patching. (Of course, since you pretty much
invented monkey-patching, you may feel differently. :-)

[1st1]

Rather tham monkey-patching, in general I recommend just copying some
code from the asyncio library and calling that. In this case you'd be
copying a tiny bit of code from create_connection(). You'd still be
calling an internal API, _make_ssl_transport(), but your code would
still be less likely to change when some part of the asyncio library
changes than with monkey-patching.

But this kind of defeats the purpose of pluggable event loop etc. I can't implement all asyncio private APIs for uvloop. Once you start using that, your code can't run on uvloop or any other asyncio implementation.

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?

That's essentially what I wanted loop.wrap_socket to do (see msg269519)

[jimfulton]

:)

I'm not a fan of monkey patching code in production, unless the code I'm monkey patching is code I control. (And since releasing code now is a lot easier than it used to be, I have much less occasion to monkey-patch code I control.)

(I'm a big fan of and am also terrified by gevent and I generally avoid it's use of monkey patching.)

[jimfulton]

I'd still like to find a way to handle already accepted server sockets.

Can we decide on either:

• a server_side flag to create_connection or

• A new interface for handling an already accepted socket? I would
  call this handle_connection, but I'll take any name. :)

I'm fine with and willing to implement either alternative.

[gvanrossum]

I like the new method better. Submit away!

--Guido (mobile)

[jimfulton]

We need an executive (Guido) decision on the name of the new API. Yury wants wrap_socket.

I don't like wrap_socket because:

• It implies that it's for wrapping client and server sockets.
  (It shouldn't be for wrapping client sockets because TOOWTDI
  and create_connection.)

• I prefer names that are about goal rather than mechanism.

But I can live with anything.

I'll defer to Yury unless Guido voices an opinion.

[1st1]

wrap_socket implies that it's for wrapping client or server sockets, but it's not. It's only for handling server sockets. Also, I prefer a name that reflects goal, not mechanism.

I think the name should be discussed over here: https://bugs.python.org/issue27392.

Why can't wrap_socket be used for wrapping client sockets? I think we can refactor asyncio to use it in 'create_connection', and maybe to accept new connections.

To my ear, 'asyncio.handle_connection' implies that asyncio will "handle" the connection, i.e. that asyncio itself would do more than just wrapping the socket and attaching a protocol instance.

[jimfulton]

Why can't wrap_socket be used for wrapping client sockets?

TOOWTDI and create_connection.

I suppose we could remove (unadvertise) this functionality from create_connection. Then we'd have code bloat because backward compatibility.

[gvanrossum]

Hold on. It's weekend. I will review this when I am near a laptop again.

--Guido (mobile)

[gvanrossum]

How about we use connect_socket() or a variant on that name? That feels similar to connect_{read,write}_pipe(), which also take a protocol_factory and an already-opened I/O object.

If it's only for server-side sockets I'd call it connect_server_side_socket() -- I don't care that the name is long, the use case is not that common. Or we could have connect_socket() with a server_side flag and a server_hostname argument, and refactor some things so that it shares most of its implementation with _create_connection_transport().

[1st1]

How about we use connect_socket() or a variant on that name? That feels similar to connect_{read,write}_pipe(), which also take a protocol_factory and an already-opened I/O object.

I like the idea to use "connect_" prefix here. How about simple "connect_accepted_socket"?

[gvanrossum]

Sounds Good To Me.

[python-dev]

New changeset 3e44c449433a by Yury Selivanov in branch '3.5':
Issue bpo-27392: Add loop.connect_accepted_socket().
https://hg.python.org/cpython/rev/3e44c449433a

New changeset 2f0716009132 by Yury Selivanov in branch 'default':
Merge 3.5 (issue bpo-27392)
https://hg.python.org/cpython/rev/2f0716009132

[1st1]

Let's keep this issue open until we have the docs updated.

[jimfulton]

Here's a daft doc update.

[jimfulton]

Does the draft doc change look OK?

[1st1]

Jim, the patch lgtm. Will merge it soon.

[jimfulton]

FTR another use case for this. :)

We have a ZEO applications where individual database users authenticate via self-signed certs. The server's SSL connection has to have this collection of certs. User CRUD operations can add and remove certs to authenticate against. SSL contexts don't provide an API for removing (or even clearing) CAs used for authentication, so we need to create new SSL contexts when the set of valid certs change. There's no way to update the SSL context used by a server, so we're wrapping accepted sockets ourselves, so we can use dynamic SSL contexts.

Some alternatives:

• Add an SSLContext API for removing or clearing CAs

• Add a Server API to update the SSL context used for new connections. (I may pursue this at some point. I spent a few minutes trying to find where a Server's SSL context is stored, but failed and can't spend more time ATM.)

[gvanrossum]

Did the patch not get merged??

On Sun, Aug 7, 2016 at 11:32 AM, Jim Fulton <report@bugs.python.org> wrote:

Jim Fulton added the comment:

FTR another use case for this. :)

We have a ZEO applications where individual database users authenticate
via self-signed certs. The server's SSL connection has to have this
collection of certs. User CRUD operations can add and remove certs to
authenticate against. SSL contexts don't provide an API for removing (or
even clearing) CAs used for authentication, so we need to create new SSL
contexts when the set of valid certs change. There's no way to update the
SSL context used by a server, so we're wrapping accepted sockets ourselves,
so we can use dynamic SSL contexts.

Some alternatives:

• Add an SSLContext API for removing or clearing CAs

• Add a Server API to update the SSL context used for new connections. (I
  may pursue this at some point. I spent a few minutes trying to find where a
  Server's SSL context is stored, but failed and can't spend more time ATM.)

----------

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue27392\>

---

[jimfulton]

idk if the patch got merged.

I just added the last comment for informational purposes. :)

Perhaps this issue can be closed.

[1st1]

Did the patch not get merged??

connect_accepted_socket was merged. The docs patch is still pending (nothing wrong with it, I just need to commit it)

[1st1]

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 ssl argument, they require ssl to be an instance of SSLContext.

Should we have the same requirement for the 'ssl' argument of newly added 'connect_accepted_socket'?

[1st1]

Also I think we should add a check, that the passed socket is AF_UNIX or AF_INET(6)

[gvanrossum]

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]

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).

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]

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]

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]

+1 restricting uvloop to AF_INET or AF_UNIX and SOCK_STREAM, at least until someone requests something else.

[1st1]

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.

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]

AFAICT this issue was resolved in python/asyncio#378. Closing this one. Thanks, Jim!

[python-dev]

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