Bug 58230

Summary: Incorrect input from ServletInputStream in ReadListener
Product: Tomcat 8 Reporter: Rossen Stoyanchev <rstoyanchev>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal    
Priority: P2    
Version: 8.0.24   
Target Milestone: ----   
Hardware: PC   
OS: Linux   

Description Rossen Stoyanchev 2015-08-10 16:06:43 UTC
I have a reproducible case where the bytes read with ServletInputStream via a javax.servlet.ReadListener are incorrect.

The code is an experiment with layering Reactive Streams over Servlet 3.1 non-blocking I/O. I apologize for not being able to create a more focused isolated example (I did try). That said it is a very basic example that hopefully with some debugging you can zero in on the issue.

To observe the failure, clone https://github.com/spring-projects/spring-reactive, then change the following read buffer size to 4096 https://github.com/spring-projects/spring-reactive/blob/master/src/main/java/org/springframework/reactive/web/servlet/HttpHandlerServlet.java#L37, then run the test https://github.com/spring-projects/spring-reactive/blob/master/src/test/java/org/springframework/reactive/web/servlet/HttpHandlerServletTomcatIntegrationTests.java.

The test sends an HTTP request with a body of 3 * 4096 bytes. The server echoes that back. However the returned body doesn't match starting at index 4096, which happens to be the start of the second chunk.

On the server requests are delegated to an HttpHandler which accepts a (Reactive Streams) Publisher for the input and returns a Publisher for the output. In the test the HttpHandler is EchoHandler which returns the input Publisher as the response Publisher effectively piping input directly to output. Underlying that are RequestBodyPublisher and ResponseBodySubscriber which adapt the Read/WriteListener to Reactive Streams.

Increasing the size of the array into which chunks are read to 4293 makes the tests pass.
Comment 1 Rossen Stoyanchev 2015-08-11 20:28:28 UTC
As the referenced project will continue to change, I've created a fork where the code will remain stable until this issue is looked at. In the original instructions simply replace "spring-projects" with "rstoyanchev" in all links.
Comment 2 Mark Thomas 2015-08-12 11:28:32 UTC
Thanks for the test case. I've got things to the point where I can run the test case in the IDE and I can see the failure. I'll start looking into what the root cause is.
Comment 3 Rossen Stoyanchev 2015-08-12 12:59:22 UTC
Great, note the code is far from perfect with the EchoHandler essentially recursing (potentially endlessly) between reading and writing. I could be wrong but to my knowledge at least it's not doing anything not allowed in the Servlet API.
Comment 4 Mark Thomas 2015-08-12 18:04:06 UTC
Fixed in trunk and 8.0.x for 8.0.25 onwards.

Thanks again for the test case.
Comment 5 Remy Maucherat 2015-11-18 12:59:51 UTC
That was actually caused by what broke the NIO2 connector in bug 57799, available() had a side effect that it would cause an IO read.

The fix was to remove the available() call, but is it certain this won't cause useless Servlet invocation (like maybe call onDataAvailable but then available would still be false ? If unsure, it is now possible to call available(false) instead without causing concurrency related corruption.