-
Notifications
You must be signed in to change notification settings - Fork 896
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
Support 100-continue
header on client side
#5646
base: main
Are you sure you want to change the base?
Conversation
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.
Excellent work, @moromin! Left a few minor comments, but it's near-flawless 👍
@@ -31,8 +31,8 @@ interface HttpResponseDecoder { | |||
|
|||
InboundTrafficController inboundTrafficController(); | |||
|
|||
HttpResponseWrapper addResponse( | |||
int id, DecodedHttpResponse res, ClientRequestContext ctx, EventLoop eventLoop); | |||
HttpResponseWrapper addResponse(AbstractHttpRequestHandler requestHandler, int id, |
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.
Should we mark requestHandler
as @Nullable
and handle the null
-case properly, given that we call this method with null
in HttpClientPipelineConfigurator
?
AbstractHttpRequestHandler requestHandler() { | ||
assert requestHandler != null; | ||
return requestHandler; | ||
} |
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.
What do you think about replacing this method as handle100Continue()
, given that the caller of this method always calls requestHandler.handle100Continue()
?
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.
It would be nice if we could call it with res.handle100Continue()
, which would reduce the cognitive load on the reader.
Let me modify it.
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
// Read a HEADERS frame and validate it. | ||
readHeadersFrame(in); | ||
// Send a CONTINUE response. | ||
sendFrameHeaders(bos, HttpStatus.CONTINUE, false); |
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.
Ditto - we need to ensure the client doesn't send anything between these two steps.
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
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.
We don't need to check in.available()
after sending 100 Continue
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
Outdated
Show resolved
Hide resolved
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.
Looks good overall 👍 Left some minor comments 🙇
@Override | ||
final void resume() { | ||
assert subscription != null; | ||
subscription.request(1); |
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.
Question) Is there no need to check if the subscription has been completed before requesting?
e.g.
subscription.request(1); | |
if (!isSubscriptionCompleted) { | |
subscription.request(1); | |
} |
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.
I think that checking the state could avoid wasted resources and errors.
Let me modify it.
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
@@ -50,6 +51,10 @@ class HttpResponseWrapper implements StreamWriter<HttpObject> { | |||
|
|||
private static final Logger logger = LoggerFactory.getLogger(HttpResponseWrapper.class); | |||
|
|||
// This handler is used only when the response requires changing the state of the next request. | |||
// Otherwise, it should be null. |
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.
This is not true since it is actually always non-null except when we make an HTTP2 upgrade request
// Otherwise, it should be null. |
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java
Show resolved
Hide resolved
|
||
final int port = ss.getLocalPort(); | ||
final WebClient client = WebClient.of("h1c://127.0.0.1:" + port); | ||
client.prepare() |
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.
Can you actually validate that the returned response has a status 201? Ditto for the other tests in this class.
|
||
out.write("HTTP/1.1 100 Continue\r\n\r\n".getBytes(StandardCharsets.US_ASCII)); | ||
|
||
assertThat(in.readLine()).isEqualTo("foo"); |
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.
I guess this will block since a newline is needed
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
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.
As with the other cases, we tried to armeria/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java Line 122 in e74b028
armeria/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java Line 315 in e74b028
However, this just raises a |
I think there are two issues with it.
Let me think a bit more about how to solve this situation. |
@@ -197,7 +202,9 @@ final boolean tryInitialize() { | |||
final void writeHeaders(RequestHeaders headers) { | |||
final SessionProtocol protocol = session.protocol(); | |||
assert protocol != null; | |||
if (headersOnly) { | |||
if (shouldExpect100ContinueHeader(headers)) { |
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.
Should we raise an exception or silently ignore the header when the request body is empty?
A client MUST NOT generate a 100-continue expectation in a request that does not include content.
https://datatracker.ietf.org/doc/html/rfc9110#section-10.1.1
return true; | ||
} | ||
|
||
if (status != HttpStatus.CONTINUE) { |
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.
It seems like we should continue to send the request without expectation header if the status is 417
A client that receives a [417 (Expectation Failed)](https://datatracker.ietf.org/doc/html/rfc9110#status.417) status code in response to a request containing a 100-continue expectation SHOULD repeat that request without a 100-continue expectation, since the 417 response merely indicates that the response chain does not support expectations (e.g., it passes through an HTTP/1.0 server).
https://datatracker.ietf.org/doc/html/rfc9110#section-10.1.1-11.4
Could you fix that?
|
||
final boolean hasInvalid100ContinueResponse = !handle100Continue(nettyRes, status); | ||
if (hasInvalid100ContinueResponse) { | ||
state = State.DISCARD; |
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.
I believe we shouldn't discard the connection, as the server might provide an immediate response, which is completely valid.
Upon receiving an HTTP/1.1 (or later) request that has a method, target URI, and complete header section that contains a 100-continue expectation and an indication that request content will follow, an origin server MUST send either:
- an immediate response with a final status code, if that status can be determined by examining just the method, target URI, and header fields, or
https://datatracker.ietf.org/doc/html/rfc9110#section-10.1.1-15.1
Based on the server's response, the following scenarios apply:
- 100 status: Continue to send the body
- 417 status: Send the original request again without the expect header
- Others status cods: Discard the request body and
set thehandle the response using the standard flow.state = State.NEED_HEADERS;
to reuse the connection.
What are your thoughts on this approach?
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.
Thank you for your comments!
I didn’t check the details…
Let me confirm and modify them~
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.
Thanks! I've revised my comment, so PTAL again. 😄
#5646 (comment) |
🔵 Update the client behavior based on the comment and RFC: AggregatedHandler vs. Subscriber
Empty Content
Response Status: 100 Continue
Response Status: 417 Expectation Failed
Response Status: Others
|
And I’d like to ask you about the above armeria/core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java Line 127 in 01d7b18
However, it didn’t work fine… |
Sure
armeria/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java Line 208 in 4c1870b
@RegisterExtension
static ServerExtension server = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
...
}
};
@RegisterExtension
static ServerExtension proxy = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
sb.serviceUnder("/", (ctx, req) -> {
if (req.headers().contains(HttpHeaderNames.EXPECT)) {
return HttpResponse.of(417);
}
return server.webClient().execute(req);
});
}
}; |
Thank you for advices!! I make it send an empty data only when HTTP1 to avoid HTTP2 stream interrupt. About |
private boolean handle100Continue(HttpResponse nettyRes, HttpStatus status) { | ||
// Ignore HTTP/1.0 | ||
if (nettyRes.protocolVersion().compareTo(HttpVersion.HTTP_1_1) < 0) { | ||
return true; |
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.
We shouldn't do this here because the request body isn't sent to the server at all.
Let's remove this method completely and call res.handle100Continue(..)
so that requestHandler
can handle it.
Thread.sleep(1000); | ||
assertThat(inputStream.available()).isZero(); | ||
|
||
out.write("HTTP/1.1 417 Expectation Failed\r\n\r\n".getBytes(StandardCharsets.US_ASCII)); |
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.
We need to add the content-length header so that the client doesn't treat the following 201 response as a body.
out.write("HTTP/1.1 417 Expectation Failed\r\n\r\n".getBytes(StandardCharsets.US_ASCII)); | |
out.write(("HTTP/1.1 417 Expectation Failed\r\n" + | |
"Content-Length: 0\r\n" + | |
"\r\n").getBytes(StandardCharsets.US_ASCII)); |
@moromin, it's more complicated than I initially thought. |
Thank you! I think it is so cool! |
It's because I have to remove the response before calling |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5646 +/- ##
============================================
+ Coverage 74.05% 74.15% +0.09%
- Complexity 21253 21361 +108
============================================
Files 1850 1855 +5
Lines 78600 78899 +299
Branches 10032 10081 +49
============================================
+ Hits 58209 58507 +298
+ Misses 15686 15670 -16
- Partials 4705 4722 +17 ☔ View full report in Codecov by Sentry. |
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.
Looks good overall! Left some minor comments 🙇
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestSubscriber.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestSubscriber.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
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.
Functionally looks good to me! Thanks! 🙇 👍 🙇
core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestSubscriber.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java
Outdated
Show resolved
Hide resolved
@@ -171,8 +173,14 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
} | |||
keepAliveHandler.onReadOrWrite(); | |||
|
|||
if (state == State.DISCARD_DATA_OR_TRAILERS && msg instanceof HttpResponse) { |
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.
Question) Just imagining scenarios, but if the 417 request included a trailer in the response could the response for the repeated request be corrupted?
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.
The LastHttpContent
that has the trailing headers will be ignored.
core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java
Outdated
Show resolved
Hide resolved
failAndReset(new IllegalArgumentException( | ||
"an empty content is not allowed with Expect: 100-continue header")); |
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.
Do we need to reset the session or stream? When the exception is raised nothing is written yet.
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.
That's a good point. Fixed to do this check before tryInitialize
and call failRequest
instead of failAndReset
.
if (res.needs100Continue()) { | ||
if (status == HttpStatus.CONTINUE) { | ||
res.resume(); | ||
} else if (status == HttpStatus.EXPECTATION_FAILED) { | ||
// Remove the response and do not close the HttpResponseWrapper in order not to | ||
// close the original response. | ||
removeResponse(resId++); | ||
if (res.repeat()) { | ||
state = State.DISCARD_DATA_OR_TRAILERS; | ||
// TODO(minwoox): reset timeout | ||
} else { | ||
// session is not acquirable. | ||
state = State.DISCARD; | ||
} | ||
return; | ||
} else { | ||
res.discardRequestBody(); | ||
} | ||
} | ||
res.startResponse(); |
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.
Question) Could we move this logic to HttpResponseWrapper
? It is unlikely that 100-continue will be implemented differently for a session protocol. It would be nice if we could delete the duplicate code in Http{1,2}ResponseDecoder
and handle one place.
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.
I had no choice because I had to call removeResponse
when EXPECTATION_FAILED
is received.
However, we have decided not to implement the repeat so I can place the logic in AbstractHttpRequestHander
now.
@moromin Had a chat with the team and we've decided not to implement
|
resume(); | ||
// TODO(minwoox): reset the timeout | ||
} else { | ||
// We do not retry the request when HttpStatus.EXPECTATION_FAILED is received |
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.
Q) If the request was an improper one, wouldn't it be better to raise an exception such as ExpectionFailedException
? The exception could be used when implementing retrying logic.
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.
That's a good suggestion. 👍
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.
On second thought, I'm not sure if it's a good idea. We don't raise an InternalServerException
when we receive 500. Can't we just let the user handle the response depending on the response status?
Motivation:
Expect: 100-continue header is supported on server-side, but not on client-side.
On client-side, it should initiate sending the body the body after
100 Continue
response received if the header is set.Modifications:
NEEDS_100_CONTINUE
state inAbstractHttpRequestHandler
to manage the client behavior:Expect: 100-continue
is set.100 Continue
response received.AbstractHttpRequestHandler
toHttpResponseWrapper
as a field to change the state in the request handler from the decoders.HttpClientExpect100HeaderTest
to the behavior for each protocol/handler.Result:
Expect: 100-continue
on the client-side. #5394