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

Refactor to introduce a sans-I/O layer? #466

Closed
aaugustin opened this issue Sep 1, 2018 · 13 comments · Fixed by #676
Closed

Refactor to introduce a sans-I/O layer? #466

aaugustin opened this issue Sep 1, 2018 · 13 comments · Fixed by #676

Comments

@aaugustin
Copy link
Member

https://sans-io.readthedocs.io/how-to-sans-io.html

Bonus: write curio- or trio-compatible versions of the main APIs?

@aaugustin aaugustin added this to the someday milestone Sep 1, 2018
@aaugustin
Copy link
Member Author

Backpressure may be a bit tricky to handle.

Sans-I/O requires an implementation that is "defined entirely in terms of synchronous functions returning synchronous results, and that does not block or wait for any form of I/O."

Probably the best way to tell the I/O layer to stop the flow of input bytes without infringing these requirements is to send stop / resume events in the output stream.

Conversely, there should be a way to let the I/O layer tell the protocol to stop the flow of output bytes.

@aaugustin
Copy link
Member Author

Websockets needs a way to reassemble incoming data chunks and read lines (for parsing the handshake) or N bytes (for parsing frames). asyncio.StreamReader and ohneio implement this part.

@aaugustin
Copy link
Member Author

hyper-h2 simply attempts to parse HTTP2 frames, giving up if there isn't enough data and retrying when more data is pushed. This results in repeated work: the header is re-parsed every time.

@aaugustin
Copy link
Member Author

Another concern is that WebSockets specifies some TCP-specific behavior, mostly related to half-closing and closing connections. This behavior cannot be encoded into the abstraction of a sans-I/O protocol.

@aaugustin aaugustin changed the title Refactor to introduce a sans-I/O layer Refactor to introduce a sans-I/O layer? Sep 2, 2018
@aaugustin
Copy link
Member Author

The websockets.framing module is already almost sans-I/O compliant: dropping the @asyncio.coroutine decorator (which mostly serves as documentation) and injecting reader = ohneio.read and writer = ohneio.write should do the job.

The websockets.http module doesn't support injecting reader but that would be easy to change.

Then, with some surgery, about half of websockets.protocol could be extracted in the same way.

We'd end up with the protocol logic split across a sans-I/O part and a with-I/O part, which may or may not be easier to maintain. I don't remember bugs in the would-be sans-I/O part, all the complexity is in the with-I/O part...

@pgjones
Copy link

pgjones commented Sep 23, 2018

There is a wsproto library that is already sans-I/O that could be helpful here.

@aaugustin
Copy link
Member Author

aaugustin commented Sep 23, 2018

Yes, I'm aware of wsproto.

It can be an inspiration for designing the API. Similar APIs will make it easier for third party integrators to support either websockets or wsproto (like uvicorn does).

I considered depending on the third-party implementation in wsproto and decided it wouldn't be a good trade-off.

websockets predates wsproto by more than three years. I'd rather continue building on the code base that I know very well and that received lots of production use than discover a new set of behaviors.

My problem isn't about writing a Sans-I/O implementation. I almost have one, as explained in the previous comment. It's more about plugging that implementation in the high-level APIs and determining whether this is an improvement — including in terms of performance.

Currently websockets doesn't have any dependencies. That's a good thing. It gives me complete control over behavior and performance. For example, websockets is more than 30 times faster than wsaccel, which wsproto uses, for masking 10kB of data:

>>> import timeit
>>> timeit.timeit(
...     """apply_mask(data, mask)""",
...     """from websockets.speedups import apply_mask; data = b'abcdefghij' * 1024; mask = b'xyzt'; """,
... )
0.46926557800000523
>>> timeit.timeit(
...     """masker.process(data)""",
...     """from wsaccel.xormask import createXorMasker; data = b'abcdefghij' * 1024; masker = createXorMasker(b'xyzt'); """,
... )
15.354961824000014
>>> 15.354961824000014 / 0.46926557800000523
32.721261784089016

EDIT: to be completely fair, wsaccel is faster for messages shorter than about 50 bytes. But xormask speed is negligible for such small messages compared to everything else Python does for each message, like round-tripping to the event loop.

@lgrahl
Copy link
Collaborator

lgrahl commented Sep 23, 2018

Do I understand correctly that this is primarily about being able to use a protocol without tightly binding it to some specific source or sink (e.g. file descriptors) and secondarily about the issue with red/blue functions? (Haven't watched the talk yet but it's now on my to do list.)

@aaugustin
Copy link
Member Author

Yes, these are the main design principles. https://sans-io.readthedocs.io/ is a reasonably short read.

@aaugustin aaugustin removed this from the someday milestone Jun 22, 2019
@aaugustin
Copy link
Member Author

The low-level API I'd like to expose should look like this: https://aioquic.readthedocs.io/en/latest/http3.html /Cc @jlaine

@jlaine
Copy link

jlaine commented Jul 7, 2019

If you want a more mature example I recommend looking at h11 and h2.

h2 is interesting in the way it handles backpressure: the API consumer "acknowledges" data.

Ping @sethmlarson ;)

@aaugustin
Copy link
Member Author

I'll take a look, thanks!

@aaugustin
Copy link
Member Author

I wrote about the implementation strategy here: https://fractalideas.com/blog/sans-io-when-rubber-meets-road/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants