Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HttpClientWithTomcatTest flaky test. #2903

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

pderop
Copy link
Member

@pderop pderop commented Sep 19, 2023

This is a new attempt to fix the flaky test from HttpClientWithTomcatTest.testIssue2825, which was still sometimes getting some Premature Close Exception.

To summarize:

  • The fix from Request for read interest when channel is unwritable while HttpClient.send(Mono) is used #2864 significantly reduces the probability to get premature close exceptions when Tomcat sends an early final response 400 bad request while the client is still writing a large request body. Did a benchmark using a real network, it works well; no premature close exceptions.
  • However, on localhost, and especially on the Github CI, the fix only alleviate the problem but we can't avoid it all the time: sometimes, once the server has sent the 400 bad request, and once it has read up to 2 MB of extra request body, it may then close the connection using a TCP/RST, so in this case, if the client has not yet consumed the 400 bad request, it may miss it (when receiving the TCP/RST, the client OS may drop any unread data).
  • The test is now doing his best in order to let the client have time to consume the 400 bad request, and if you disable the patch, the test will reproduce the issue described in Misleading reactor.netty.http.client.PrematureCloseException: Connection prematurely closed BEFORE response #2825.

Did the following fixes:

  • Fixed thePayloadSizeServlet which was not counting correctly the number of bytes received.
  • Fixed test http request PAYLOAD size: when the max payload size is exceeded, the test should at least write some extra bytes >= 2 MB, which is the value for maxSwallowSize on Tomcat. Else, the test would pass even without applying the fix from Request for read interest when channel is unwritable while HttpClient.send(Mono) is used #2864
  • Reduce the SO_RCVBUF on the Tomcat server, to reduce the possibility that the client is writing while the server is closing the connection (with a small Tomcat SO_RCVBUF, the client will likely be blocked on socket writing while the server is closing the connection.
  • To avoid racy situation when the client can't read the response before the socket is closed with a TCP/RST or when the client is getting a broken pipe while writing, the testIssue2825 is now using a RetryWhen strategy in order to retry the request a couple of times before failing. This is needed, else the test may remain flaky when running on the CI.

@pderop pderop added the type/test A general test label Sep 19, 2023
@pderop pderop added this to the 1.0.37 milestone Sep 19, 2023
@pderop pderop self-assigned this Sep 19, 2023
@pderop pderop marked this pull request as draft September 19, 2023 15:38
@pderop pderop force-pushed the 1.0.x-fix-tomcat-flaky-test branch 4 times, most recently from 3c330b3 to ab870d0 Compare September 20, 2023 08:46
@pderop pderop marked this pull request as ready for review September 20, 2023 13:30
@pderop
Copy link
Member Author

pderop commented Sep 20, 2023

I think that the errors from the windows checks are not related.

@pderop pderop marked this pull request as draft September 20, 2023 20:50
@pderop pderop force-pushed the 1.0.x-fix-tomcat-flaky-test branch 6 times, most recently from f611bdd to c272232 Compare September 21, 2023 20:55
Adapted maxSwallowSize, and PAYLOAD size.
Do not configure Tomcat socket rcvbuf size anymore.
Reduce HttpClient sndbuf size.
400 Bad Request is not sent with chunked encoding.

temporary testing

temporary testing
@pderop pderop marked this pull request as ready for review September 22, 2023 08:56
@pderop
Copy link
Member Author

pderop commented Sep 22, 2023

just applied the suggested changes.

@pderop pderop merged commit 78cae2f into reactor:1.0.x Sep 22, 2023
9 checks passed
pderop added a commit that referenced this pull request Sep 22, 2023
pderop added a commit that referenced this pull request Sep 22, 2023
@pderop
Copy link
Member Author

pderop commented Sep 22, 2023

@violetagg , thanks for your review.

pderop added a commit to pderop/reactor-netty that referenced this pull request Sep 25, 2023
pderop added a commit that referenced this pull request Sep 25, 2023
Fix cross-site scripting (XSS) vulnerability github security alert in the TomcatServer class.
See https://github.com/reactor/reactor-netty/security/code-scanning/9
@violetagg violetagg changed the title Fix HttpClientWithTomcatTest flaky test. Fix HttpClientWithTomcatTest flaky test. Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/test A general test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants