Iteratively encode JSON responses

[clokep]

This uses a producer to encode JSON in chunks to respond to requests. Fixes #6998.

This unfortunately rolls back a bit of the abstraction since respond_with_json can't call respond_with_json_bytes anymore... further it seems that respond_with_json_bytes is reasonable called in a couple of other places. 😢

This also uses private members from canonicaljson which is kind of dirty, but wanted to put it up as a proof of concept.

[clokep]

@auscompgeek Brought up an interesting point which is if Twisted coalesces any of this data together. Using curl --raw I'm able to see that it doesn't, so some bits of JSON become really wasteful (e.g. { / , / : all end up taking 4 bytes: e.g. 1\r\n{).

This is easy enough to fix in the code, although I don't know of a best practice of how large a chunk should be.

[erikjohnston]

Is this true? We're' not awaiting here so the reactor should not be able to spin?

[richvdh]

I think it's cut-and-pasted from elsewhere; but I seem to remember that it is true that this can be re-entrant.

[clokep]

This was originally cut-and-pasted from the NoRangeStaticProducer and then tweaked a bit to be more readable.

[clokep]

I added some logging and I actually wasn't able to cause this to be re-entrant?

Anyway the question of "how does the reactor spin?" is convoluted, but I think I found the answer:

• Calling registerProducer on the Request actually registers it with the HTTPChannel
• The HTTPChannel then converts the IPullProducer to an IPushProducer.
• It also calls startStreaming which generates a cooperator
• The cooperated task (_PullToPush._pull) yields after each call to resumeProducing to give up control to the reactor.

The result is that in one reactor "tick" this should generate <~1 KB of data.

[erikjohnston]

Ok, right interesting. Since this function is not a coroutine I don't think this can yield back to the reactor half way through the function

[clokep]

Right, it "yields" when the function runs off, NOT in the middle of the function.

Do you think any changes are needed here?

[erikjohnston]

I guess I still don't really see what this comment is trying to convey. write may or may not cause the reactor to wake up immediately, and I don't see why we are trying to highlight that.

If we want to highlight that this function may be re-entrant, it'd be good to mention what the code is doing to make that safe (I guess something to do with the fact that the write is the last thing we do in the function, and its only during write that we can become re-entrant?)

[clokep]

I believe it is important that you check if self._request still exists since something could have called stopProducing.

Maybe it isn't useful to clarify -- I can remove the part of the comment if you'd like (or maybe move it above the self._request check?)

[erikjohnston]

Saying that at by the check I think would be best

[clokep]

@erikjohnston I clarified a bunch of comments! Let me know if it is better now!

[erikjohnston]

@auscompgeek Brought up an interesting point which is if Twisted coalesces any of this data together. Using curl --raw I'm able to see that it doesn't, so some bits of JSON become really wasteful (e.g. { / , / : all end up taking 4 bytes: e.g. 1\r\n{).

This is easy enough to fix in the code, although I don't know of a best practice of how large a chunk should be.

I think 1-4 KB is a standard amount?

[clokep]

@auscompgeek Brought up an interesting point which is if Twisted coalesces any of this data together. Using curl --raw I'm able to see that it doesn't, so some bits of JSON become really wasteful (e.g. { / , / : all end up taking 4 bytes: e.g. 1\r\n{).
This is easy enough to fix in the code, although I don't know of a best practice of how large a chunk should be.

I think 1-4 KB is a standard amount?

I chose 1 KB arbitrarily, but it is configurable. 😄 This seems to conveniently fix the tests too.

[clokep]

See matrix-org/python-canonicaljson#29 for the changes to canonicaljson necessary for this.

[clokep]

Looks like the sytests are failing since they're not installing an updated canonicaljson package.

I guess one question here is -- do we care to allow people to use an older version of canonicaljson? My intention was to bump it in this PR once matrix-org/python-canonicaljson#29 was done.

[clokep]

This should be test-able now that we released an updated canonicaljson package.