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
Implement #8057 103 Early Hint #8058
Conversation
@lorban can you look at why h3 is failing. |
@gregw I see no H3 test failure in the PR's build, only a checkstyle error. What am I overlooking? |
@lorban have you run org.eclipse.jetty.http.client.IntermediateResponseTest#test103EarlyHint ? |
jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
Outdated
Show resolved
Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
...tp3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
...p-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java
Outdated
Show resolved
Hide resolved
@lorban @sbordet I've updated for review feedback. |
Hmmm and I've just had a flake of the 102 test for UNIX_DOMAIN as well... also not reproducible :( |
@sbordet what does the client do if it is still processing the 102/103 response and the final (or next) response turns up? On the server, we don't wait for anything from the client before sending the next response, but if I put in a 10ms sleep between the responses then I don't get any flakes. Either we are writing over the responses or the client is processing the next one before the last is finished? |
@sbordet I think the race is in the client. If I put the 10ms sleeps in the server I pass every time. However, if I put a 100ms sleep in the onSuccess handling for the 1xx responses, then I fail more often than not, even for HTTP. |
* Send a 103 response as per <a href="https://datatracker.ietf.org/doc/html/rfc8297">RFC8297</a> | ||
* This method is called by sendError if it is passed 103. | ||
* | ||
* @throws IOException if unable to send the 102 response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if unable to send the 102 response
+ if unable to send the 103 response
All tests in |
Added test harness for intermediary responses.
Implemented 103 early hint, but is failing for HTTP>=2
Fixed H2 Attempted fix H3
Fixed H2 Attempted fix H3
fixed checkstyle
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
updates from review
@sbordet @lorban The flaky issue is somewhere below the generation of the response in the server and the parsing of it in the client. I added some println's and for a good run I see:
So each generated response is parsed (startResponse) in the client. But for a bad run I see:
So all the responses are generated, but one of them is not received. It feels like something might be clearing a buffer when it should not? @lorban can you focus on the H2/H3 flakes and I'll keep looking at h1 |
updates from review
bfa8c65
to
9d40cd4
Compare
oops sorry forced pushed after a rebase |
Found (but not fixed) flake.
// TODO this can clean a pipelined response after a 1xx | ||
if (LOG.isDebugEnabled()) | ||
LOG.debug("Discarding unexpected content after response: {}", networkBuffer); | ||
networkBuffer.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed flake. @sbordet please review this commit!
@gregw I can make That's strange... |
@gregw looking |
Improved implementation on the client-side. Introduced HttpStatus.isInterim() for 1XX codes that are not 101. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Implementing #8057 Early hints