tornado icon indicating copy to clipboard operation
tornado copied to clipboard

Using loop.sock_recv_into & loop.sock_sendall

Open jakirkham opened this issue 4 years ago • 5 comments

In PR ( https://github.com/tornadoweb/tornado/pull/2193 ), there was some discussion of moving away from file descriptor based interfaces in the future. One option that came up was using sock_recv_into. Included a relevant quote from ( https://github.com/tornadoweb/tornado/pull/2193#issuecomment-350333882 ) in the thread below. Now that Tornado is Python 3 only and uses asyncio, am curious if this is something that makes sense to revisit.

One thing I've been thinking about for the future of IOStream (after all IOLoops are backed by asyncio) is to use less file-descriptor-centric interfaces (for windows compatibility and likely performance). This would get in the way of that since asyncio doesn't have something that could be used to implement |read_into|. I added sock_recv_into() in Python 3.7: https://docs.python.org/3.7/library/asyncio-eventloop.html#low-level-socket-operations

(it's implemented for POSIX and Windows)

jakirkham avatar Jan 29 '21 17:01 jakirkham

Should add uvloop also supplies sock_recv_into. So if we are able to use sock_recv_into from asyncio, it should make it easy to benefit from uvloop here as well (when it is enabled).

jakirkham avatar Jan 29 '21 20:01 jakirkham

A related question would be whether sock_sendall can be used

jakirkham avatar Jan 30 '21 01:01 jakirkham

Tornado still supports Python 3.6, so we can't require sock_recv_into or sock_sendall (which were added in Python 3.7) yet. But we could use them with suitable fallbacks (and 3.6 reaches EOL in December 2021, so after that we can stop worrying about compatibility with it).

But other than that consideration, I'm open to reworking IOStream to use the asyncio event loop more directly and use more modern interfaces when available. Hopefully this would improve performance and/or let us get rid of some of the more complex buffer-management code we have today. (whoever does this work should also do the appropriate performance testing, but it sounds like you're already doing that in dask/distributed#4443).

bdarnell avatar Feb 02 '21 01:02 bdarnell

Yep that seems reasonable.

Makes sense. Do you have any particular thoughts on how we go about this? Guessing you may have given this more thought than we have at this point.

jakirkham avatar Feb 02 '21 01:02 jakirkham

Not really - the last major changes to IOStream were made by dask folks IIRC.

Ultimately, if we move all of IOStream to asyncio-native interfaces, most of the current code (all the handle methods for example) become obsolete. And doing things incrementally (say with read_into using asyncio while other reads use the old paths) seems like it might be more difficult than starting from scratch. So I think I'd at least start with the from-scratch option as a proof of concept. Can you cleanly implement the IOStream interfaces on top of sock_recv_into and sock_sendall? It might just work or there might be subtle issues (which would hopefully be uncovered by Tornado's test suite)

bdarnell avatar Feb 02 '21 02:02 bdarnell