Switch the JSON byte producer from a pull to a push producer

[clokep]

Per some conversation with @erikjohnston and @richvdh this might be more efficient as we can hopefully fill more of the TCP buffer in the kernel before giving up control of the reactor.

The _ByteProducer will create HTTP chunks of 1 KB, but now it should continue writing chunks until told to stop.

[clokep]

https://twisted.readthedocs.io/en/twisted-20.3.0/core/howto/producers.html had some relevant information that was useful.

In particular it refers to an example: https://github.com/twisted/twisted/blob/trunk/docs/core/examples/streaming.py

[clokep]

This definitely looks like an infinite loop, but I don't think it is (see a previous discussion about this description here). I think when we call write we give up control to the reactor which might call pauseProducing before control is returned to resumeProducing.

[clokep]

When you call write() you go through the Request -> HTTPChannel -> iocpreactor.tcp.Connection (this is HTTPChannel.transport) -> iocpreactor.abstract.FileHandle, which might cause producer.pauseProducing (which goes back through the chain in the opposite direction, essentially).

[erikjohnston]

This does mean that if we write slower than the TCP buffer drains we will write everything in one go. I don't know whether we want to put some limit to the amount written in one reactor tick? Maybe something like 64KB (or even 512KB)? A reactor tick generally takes less than 1ms, so those values would mean maxing out at ~64MB/s (or 512MB/s).

[erikjohnston]

Aha, but FileHandle has a write buffer of 64KB and doesn't actually pull from the buffer until a reactor tick, so we don't need to worry about it. The flow becomes: 1) write data, 2) pause producing is called if over 64kb of data has been written, 3) we exit the loop until the FileHandle buffer has been drained.

[clokep]

That was my understanding as well! 👍 This is some fun spelunking into Twisted.

[clokep]

I believe the commit I just pushed will fix the tests, but I'm not 100% sure the changes are "correct".

[erikjohnston]

This does mean that if we write slower than the TCP buffer drains we will write everything in one go. I don't know whether we want to put some limit to the amount written in one reactor tick? Maybe something like 64KB (or even 512KB)? A reactor tick generally takes less than 1ms, so those values would mean maxing out at ~64MB/s (or 512MB/s).

[erikjohnston]

Looking at the code this is basically what iocpreactor.abstract.FileHandle is doing, so I think we can just skip all this and call self._request.write(data) for each chunk?

[clokep]

The issue is that the http.Request calls toChunk so each time we call write it will get turned into an HTTP chunk.

The goal of this code is to avoid b"{" being emitted from the JSONProducer becoming the HTTP chunk of 1\r\n{\r\n, which is a large explosion in the streamed bytes.

[erikjohnston]

Oh gah I hate that http.Request gets in the way :/

[erikjohnston]

Can you add a comment to that effect please?

[clokep]

I expanded the comment above this section in 2afe301.

[erikjohnston]

Aha, but FileHandle has a write buffer of 64KB and doesn't actually pull from the buffer until a reactor tick, so we don't need to worry about it. The flow becomes: 1) write data, 2) pause producing is called if over 64kb of data has been written, 3) we exit the loop until the FileHandle buffer has been drained.

[clokep]

This appears to be a follow-on to #8013.