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

Add loop.connect_accepted_socket #71579

Closed
jimfulton mannequin opened this issue Jun 26, 2016 · 44 comments
Closed

Add loop.connect_accepted_socket #71579

jimfulton mannequin opened this issue Jun 26, 2016 · 44 comments
Assignees
Labels
topic-asyncio type-feature A feature request or enhancement

Comments

@jimfulton
Copy link
Mannequin

jimfulton mannequin commented Jun 26, 2016

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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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. :)

    @jimfulton jimfulton mannequin added topic-asyncio type-feature A feature request or enhancement labels Jun 26, 2016
    @1st1
    Copy link
    Member

    1st1 commented Jun 26, 2016

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Member

    1st1 commented Jun 29, 2016

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jun 29, 2016

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

    @jimfulton
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Member

    1st1 commented Jun 29, 2016

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jun 29, 2016

    > 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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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

    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Member

    1st1 commented Jun 29, 2016

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jun 29, 2016

    :)

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jul 9, 2016

    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
    Copy link
    Member

    I like the new method better. Submit away!

    --Guido (mobile)

    @jimfulton
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jul 9, 2016

    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
    Copy link
    Member

    1st1 commented Jul 9, 2016

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jul 9, 2016

    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
    Copy link
    Member

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

    --Guido (mobile)

    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Member

    1st1 commented Jul 11, 2016

    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
    Copy link
    Member

    Sounds Good To Me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 12, 2016

    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
    Copy link
    Member

    1st1 commented Jul 12, 2016

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

    @jimfulton
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jul 14, 2016

    Here's a daft doc update.

    @jimfulton
    Copy link
    Mannequin Author

    jimfulton mannequin commented Jul 18, 2016

    Does the draft doc change look OK?

    @1st1
    Copy link
    Member

    1st1 commented Jul 19, 2016

    Jim, the patch lgtm. Will merge it soon.

    @1st1 1st1 self-assigned this Jul 19, 2016
    @jimfulton
    Copy link
    Mannequin Author

    jimfulton mannequin commented Aug 7, 2016

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Member

    1st1 commented Aug 8, 2016

    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
    Copy link
    Member

    1st1 commented 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 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
    Copy link
    Member

    1st1 commented Aug 9, 2016

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

    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Member

    1st1 commented Aug 9, 2016

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    jimfulton mannequin commented 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
    Copy link
    Mannequin Author

    jimfulton mannequin commented Aug 9, 2016

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

    @1st1
    Copy link
    Member

    1st1 commented 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.

    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
    Copy link
    Member

    1st1 commented Oct 6, 2016

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

    @1st1 1st1 closed this as completed Oct 6, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented 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

    @1st1 1st1 changed the title Add a server_side keyword parameter to create_connection Add loop.connect_accepted_socket Nov 7, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants