Optimize how SSE data is written

[pR0Ps]

This is a followup of #819 (comment)

The SseConnection now writes queued events directly to the response's output stream from the connection-handling thread itself. Previously each SSE connection has its own virtual thread that would create and push the events into a pipe for the actual connection-handling thread to read from.

Per SSE connection, this new method saves the creation of a virtual thread, a PipedOutputStream/PipedInputStream pair and their internal buffer, and the overhead of having to push all the SSE data across a thread boundary.

Additionally, because the SseConnection now handles writing the data to the output stream itself and can handle flushing it as needed, the HttpResponseOutputStream no longer needs to flush the output stream after every buffer read just in case the data it was sending was part of an SSE stream.

To support this change, the HttpResponseStreamWriter interface was added to allow HttpResponse bodies to be streamed incrementally and dynamically chunked using the new ChunkedOutputStream rather than only being able to be read fixed-size chunks from an InputStream.

Note that because the pipe buffer is now gone, event buffering in the SseConnection occurs entirely in the queue. To compensate for this, the QUEUE_CAPACITY was increased from 16 to 64.

[TBlueF]

Should the buffer have some threshold or max size that will flush it or end the chunk automatically if exceeded?

[pR0Ps]

I didn't add one because the HttpResponseStreamWriter and SseConnection either write an entire chunk (doesn't use the buffer at all) or explicitly flush the stream after every event is written so the buffer will never grow out of control.

With that being said, I do see how this could be an issue in the future if something else starts using this and doesn't make sure to periodically flush it.

I have 2 ideas for how I could solve this:

1. Add a max chunk size limit and auto-flush in the write methods if the internal buffer gets larger than that.
2. Since the HttpResponseStreamWriter always writes an entire chunk and the SseConnection could be pretty trivially refactored to internally buffer its events, I could remove the internal buffer and write methods from the ChunkedOutputStream entirely and enforce that an entire chunk must be written at once.

[TBlueF]

It's true that it is not an issue with how the class is being used right now, but I like having classes like this as individually "safe" as possible. It makes potential future changes safer and there is also potential use by addons :D

I think i personally would go with 1, but i am also totally fine with 2 👍
So i'll leave that up to you :)

[pR0Ps]

I've pushed a commit that does 1

[TBlueF]

Since we convert the body into a HttpResponseStreamWriter anyways, maybe we could do this here instead?
Aka, do this:

Suggested change

	private @Nullable InputStream body;
	/**
	* If set, takes over writing this response's body directly to the connection's output-stream
	* instead of reading it from {@link #body}.
	* Used for responses that push data over time like Server-Sent Events.
	*/
	private @Nullable HttpResponseStreamWriter streamWriter;
	private @Nullable HttpResponseStreamWriter body;

and then do the conversion in the setBody() methods, and add a new setBody(HttpResponseStreamWriter body)..

Just a thought, i might be missing some obvious problem with this, but if there is no problem i think i would prefer that :)

[pR0Ps]

I considered something like this but I ran into the issue where that if the body InputStream was automatically wrapped up as a HttpResponseStreamWriter we would still need to keep a reference to the original body so it could be closed by HttpResponse.close(), even if the response was never used. The body could be closed from within the lambda, but that's only run if the response is used.

I'm not a huge fan of the logic in HttpResponseOutputStream that has to choose between using response.getBody() or response.getStreamWriter() so I'll take another shot at encapsulating the two different body types in the HttpResponse and providing a consistent interface for the HttpResponseOutputStream to use.

[TBlueF]

Oh! I see the issue 🤔
I knew i was missing something :D

Maybe make HttpResponseStreamWriter closable as well, then there the implementation could keep the reference if needed to delegate the closing?

[pR0Ps]

I added 2 commits related to this:

• Encapsulate body+streamwriter differences within HttpResponse is my take on keeping the body and streamWriter attributes separate, but making the HttpResponse deal with it internally.
• Always convert HttpResponse.body to a HttpResponseStreamWriter is what you suggested above.

The second commit can either be squashed or reverted depending on what implementation you think is better.

[TBlueF]

Love it! Thanks!