Don't block the event loop when reading inputstream.

[graemerocher]

Fixes #6100

[jameskleeh]

Wouldn't this lead to OOM?

[graemerocher]

InputStream is not thread safe, so I don't see an alternative

[jameskleeh]

Then I don't think this is an improvement

[graemerocher]

Provide a reproducer for your OOM theory in that case. "I don't think" is not a valid argument opposing a bug fix

[jameskleeh]

I will try to create the scenario. In the meantime, relax with your tone

[jameskleeh]

Basically my thoughts are that if the stream is not read, etc.. we shouldn't buffer the data as that will lead to OOM exceptions. It's my understanding that in order to not buffer the event loop thread has to wait for the consumer to read the data.

[graemerocher]

Feel free to amend the PR if you find a solution that is both thread safe and doesn't need buffering. Right now the current implementation is blocking the event loop

[jameskleeh]

It's only blocking the event loop if the data isn't read. If the data isn't read with this change it could lead to OOM. I see this as a documentation issue

[graemerocher]

Also note that we are already buffering the data in FlowControlHandler so I don't buy the OOM argument, if you can provide a test where you can trigger an OOM then amend the test to demonstrate that.

[graemerocher]

@jameskleeh that is not true, the test reads the data and it is blocking the event loop

[jameskleeh]

Yes it is read after it requires a thread from the event loop. If it is read before that it would not be an issue

[graemerocher]

Set a break point on the code that writes to the output stream and you will note that all writes to the output stream (a blocking API) occur on the event loop thread without my change. After my change they occur on the I/O thread.

If that isn't blocking the event loop I don't know what is 🤷‍♂️

[graemerocher]

Basically my thoughts are that if the stream is not read, etc.. we shouldn't buffer the data as that will lead to OOM exceptions. It's my understanding that in order to not buffer the event loop thread has to wait for the consumer to read the data.

I'm confused by what you are saying here, looking at the code prior to my change, the InputStream is bound and immediately on the same thread the code subscribes to the processor regardless whether any data is actually read from the InputStream:

micronaut-core/http-server-netty/src/main/java/io/micronaut/http/server/netty/binders/InputStreamBodyBinder.java

Lines 79 to 80 in 8da7f72

	PipedInputStream inputStream = new PipedInputStream(outputStream);
	processor.subscribe(new CompletionAwareSubscriber<ByteBufHolder>() {

At this point the PipedOutputStream will be buffering the data anyway so I have no idea what you are referring to with regards to buffering being a problem with the change, since buffering is already happening within the PipedOutputStream

[jameskleeh]

When you write to a PipedOutputStream, it blocks the current thread to wait for the reader to read the data. See the awaitSpace method in PipedInputStream

[graemerocher]

Well exactly, so why are we writing to it on the event loop thread?

[jameskleeh]

Perhaps I'm misunderstanding something here so please tell me if so

If we should not buffer data then the event loop thread has to wait for the data to be read before requesting more. If the server is continuously reading data from the client and the data isn't being read then it has to go somewhere (buffered), or block.

[graemerocher]

Indeed you have to block somewhere, but that somewhere should not be the event loop which is what is currently being blocked.

[jameskleeh]

I was able to reproduce the OOM in my example app before the latest changes

The request is hung with the latest changes (JDK 11). Could be some daemon/cache/etc issue on my machine

https://github.com/jameskleeh/delay-read-stream

Changing version back to 3.0.1 makes it work again