Support `100-continue` header on client side #5646

moromin · 2024-04-27T15:11:15Z

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:

Add NEEDS_100_CONTINUE state in AbstractHttpRequestHandler to manage the client behavior:
- Write the headers only when Expect: 100-continue is set.
- Write the body and trailers when 100 Continue response received.
- Abort the request when the response except for the above received.
Add the similar changes to HTTP response decoders.
- Add AbstractHttpRequestHandler to HttpResponseWrapper as a field to change the state in the request handler from the decoders.
Create HttpClientExpect100HeaderTest to the behavior for each protocol/handler.

Result:

Closes Respect Expect: 100-continue on the client-side. #5394

github-actions · 2024-04-27T16:37:11Z

🔍 Build Scan® (commit: `e77bfa3`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/ea6qnwzzwg6us
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/n3xktsnngipbs
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/57d3a6xjfdmdm
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/l72ieseyemlxo
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/7kwkqedninrey
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/zdpcsvejtiqxi
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/7ec7q7wab4mfs
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/6wl77ythfvsly

trustin

Excellent work, @moromin! Left a few minor comments, but it's near-flawless 👍

trustin · 2024-04-30T06:14:10Z

core/src/main/java/com/linecorp/armeria/client/HttpResponseDecoder.java

@@ -31,8 +31,8 @@ interface HttpResponseDecoder {

    InboundTrafficController inboundTrafficController();

-    HttpResponseWrapper addResponse(
-            int id, DecodedHttpResponse res, ClientRequestContext ctx, EventLoop eventLoop);
+    HttpResponseWrapper addResponse(AbstractHttpRequestHandler requestHandler, int id,


Should we mark requestHandler as @Nullable and handle the null-case properly, given that we call this method with null in HttpClientPipelineConfigurator?

trustin · 2024-04-30T06:16:01Z

core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java

+    AbstractHttpRequestHandler requestHandler() {
+        assert requestHandler != null;
+        return requestHandler;
+    }


What do you think about replacing this method as handle100Continue(), given that the caller of this method always calls requestHandler.handle100Continue()?

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

trustin · 2024-04-30T06:20:11Z

core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

+                    // Read a HEADERS frame and validate it.
+                    readHeadersFrame(in);
+                    // Send a CONTINUE response.
+                    sendFrameHeaders(bos, HttpStatus.CONTINUE, false);


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

…decoder

…eWrapper

trustin

We don't need to check in.available() after sending 100 Continue

core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

jrhee17

Looks good overall 👍 Left some minor comments 🙇

jrhee17 · 2024-05-03T06:50:00Z

core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestSubscriber.java

+    @Override
+    final void resume() {
+        assert subscription != null;
+        subscription.request(1);


Question) Is there no need to check if the subscription has been completed before requesting?

e.g.

Suggested change

subscription.request(1);

if (!isSubscriptionCompleted) {

subscription.request(1);

}

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

jrhee17 · 2024-05-03T07:13:40Z

core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java

@@ -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.


This is not true since it is actually always non-null except when we make an HTTP2 upgrade request

Suggested change

// Otherwise, it should be null.

core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java

jrhee17 · 2024-05-03T07:34:58Z

core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

+
+                final int port = ss.getLocalPort();
+                final WebClient client = WebClient.of("h1c://127.0.0.1:" + port);
+                client.prepare()


Can you actually validate that the returned response has a status 201? Ditto for the other tests in this class.

jrhee17 · 2024-05-03T07:48:29Z

core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

+
+                    out.write("HTTP/1.1 100 Continue\r\n\r\n".getBytes(StandardCharsets.US_ASCII));
+
+                    assertThat(in.readLine()).isEqualTo("foo");


I guess this will block since a newline is needed

ikhoon · 2024-05-07T05:55:44Z

core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java

@@ -233,6 +240,40 @@ final void writeHeaders(RequestHeaders headers) {
        encoder.writeHeaders(id, streamId(), merged, headersOnly, promise);
    }

+    private static boolean handleExpect100ContinueHeader(RequestHeaders headers) {


handle sounds like performing something long.

Suggested change

private static boolean handleExpect100ContinueHeader(RequestHeaders headers) {

private static boolean shouldExpect100ContinueHeader(RequestHeaders headers) {

By the way, this method seems to be replaced with:

return headers.contains(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE.toString());

ikhoon · 2024-05-07T07:26:31Z

core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java

@@ -70,20 +72,29 @@ private void apply0(@Nullable AggregatedHttpRequest request, @Nullable Throwable
            return;
        }

-        HttpData content = request.content();
+        aReq = request;
+        if (state() != State.NEEDS_100_CONTINUE) {


What do you think of using HttpRequestSubscriber if Expect: 100-continue header is set for simplicity? I don't think we need to implement support for Expect: 100-continue for all request types.

if (!ctx.exchangeType().isRequestStreaming() && shouldExpect100Continue()) { final AggregatedHttpRequestHandler reqHandler = new AggregatedHttpRequestHandler( ... }

Expect: 100-continue is usually used when a request body is large.

It is not common to use 100-continue with WebSocket.

I'm not sure if this simplification idea is a good idea. It simplifies things on our side a bit, but it may be surprising to a user when the behavior and performance characteristics change depending on whether the header exists or not.

As 100-continue is not commonly used for REST API, I wanted to keep the optimal code path for most requests.

That said, the impact on performance would be trivial with the current changes. I agree with your idea. It makes sense.

I believe we should also support for request streaming type because a user wants to send a unary POST request with ExchangeType.BIDI_STREAMING type. Let me fix this in the follow-up PR.

trustin

moromin · 2024-05-08T11:00:38Z

As with the other cases, we tried to future.join() and verify status for the HTTP1 failure case:

armeria/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

Line 122 in e74b028

void failToSendHttp1Request() throws Exception {

armeria/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

Line 315 in e74b028

void failToSendHttp1StreamingRequest() throws Exception {

However, this just raises a ResponseTimeoutException...
Am I missing something necessary in the Http1ResponseDecoder? Do you have any ideas?

minwoox · 2024-05-08T12:12:12Z

Am I missing something necessary in the Http1ResponseDecoder? Do you have any ideas?

I think there are two issues with it.

response.close() is never called, which means the aggregating future is not completed.
The test thread hangs at assertThat(in.readLine()).isNull(); because the client doesn't send any packet after that.

Let me think a bit more about how to solve this situation.

minwoox · 2024-05-08T12:27:04Z

core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java

@@ -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)) {


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

minwoox · 2024-05-08T12:38:08Z

core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java

+            return true;
+        }
+
+        if (status != HttpStatus.CONTINUE) {


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?

minwoox · 2024-05-08T12:54:22Z

core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java

+
+                        final boolean hasInvalid100ContinueResponse = !handle100Continue(nettyRes, status);
+                        if (hasInvalid100ContinueResponse) {
+                            state = State.DISCARD;


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 the state = State.NEED_HEADERS; to reuse the connection.~~ handle the response using the standard flow.

What are your thoughts on this approach?

Thank you for your comments!
I didn’t check the details…
Let me confirm and modify them~

Thanks! I've revised my comment, so PTAL again. 😄

jrhee17 · 2024-05-09T00:55:41Z

However, this just raises a ResponseTimeoutException...
Am I missing something necessary in the Http1ResponseDecoder? Do you have any ideas?

#5646 (comment)
Like pointed out in this comment, the readLine() is expecting a newline (e.g. foo\n)

… header when streaming request

moromin · 2024-05-14T16:10:42Z

🔵 Update the client behavior based on the comment and RFC:

AggregatedHandler vs. Subscriber

Handle the header like the middle ground in the discussion.
Can use the both handlers even if the header is set as below:
- AggregatedHandler
  - Handle the header as RFC.
- Subscriber
  - Ignore the header and log a warning since it is not common to use it and cannot check if the content is empty or not before sending request.

Empty Content

Throw an IllegalArgumentException.

Response Status: 100 Continue

No changes (send content after 100 received)

Response Status: 417 Expectation Failed

TODO: Implement logic to repeat the request without the header.

Response Status: Others

Process the response with the standard flow, i.e., ignore the header.

moromin · 2024-05-14T16:12:11Z

And I’d like to ask you about the above 417 case.
Client should repeat the request without the header, so I tried to implement repeat() method and call it when 417 is received.

armeria/core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java

Line 127 in 01d7b18

void repeat() {

However, it didn’t work fine…
Could you give me any advice on how to make a state transition correctly?

minwoox · 2024-05-16T02:27:49Z

However, it didn’t work fine…
Could you give me any advice on how to make a state transition correctly?

Sure

We need to write an empty data (writeData(HttpData.empty().withEndOfStream());) instead of reset here:

armeria/core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java

Line 130 in 01d7b18

reset(null);

Let's introduce another status DISCARD_DATA_OR_TRAILERS because a body might be following after 417:

try {
  switch (state) {
      case DISCARD_DATA_OR_TRAILERS:
          if (!(msg instanceof HttpResponse)) {
              return;
          }
          state = State.NEED_HEADERS;
      case NEED_HEADERS:
          ...
          final boolean hasInvalid100ContinueResponse = !handle100Continue(nettyRes, status);
          if (hasInvalid100ContinueResponse) {
              if (status == HttpStatus.EXPECTATION_FAILED) {
                  // No need to write response headers when 417 Expectation Failed is received.
                  state = State.DISCARD_DATA_OR_TRAILERS;
                  written = true;

armeria/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java

Line 208 in 4c1870b

state = State.NEED_INFORMATIONAL_DATA;

Could you also fix Http2ResponseDecoder?

armeria/core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java

Line 212 in 57187d6

res.handle100Continue(status);
Could you also add a test that uses an Armeria server as a proxy?

@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);
        });
    }
};

moromin · 2024-05-17T18:24:05Z

Thank you for advices!!

I make it send an empty data only when HTTP1 to avoid HTTP2 stream interrupt.
And I add a test using proxy, then it works fine.

About DISCARD_DATA_OR_TRAILERS status, I change the style a bit to avoid fall through of switch
But, it is so difficult to handle it….
It gets ResponseTimeout again when future.join(), but I don't know when/where it should do res.close().
(On the other hand, using ***.aggregate().thenApply(res -> assert(…)) works fine)

minwoox · 2024-05-20T02:08:07Z

core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java

+    private boolean handle100Continue(HttpResponse nettyRes, HttpStatus status) {
+        // Ignore HTTP/1.0
+        if (nettyRes.protocolVersion().compareTo(HttpVersion.HTTP_1_1) < 0) {
+            return true;


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.

minwoox · 2024-05-20T02:30:55Z

core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java

+                    Thread.sleep(1000);
+                    assertThat(inputStream.available()).isZero();
+
+                    out.write("HTTP/1.1 417 Expectation Failed\r\n\r\n".getBytes(StandardCharsets.US_ASCII));


We need to add the content-length header so that the client doesn't treat the following 201 response as a body.

Suggested change

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));

minwoox · 2024-05-20T07:14:23Z

@moromin, it's more complicated than I initially thought.
I've pushed a commit to make it work. Please take a look and let me know what you think.

moromin · 2024-05-21T07:28:17Z

Thank you! I think it is so cool!
Especially about the branching in ResponseDecoder, this one is easier to understand than RequestHandler since the processing of this header depends on the response status.

minwoox · 2024-05-21T07:38:38Z

this one is easier to understand than RequestHandler since the processing of this header depends on the response status

It's because I have to remove the response before calling repeat() so that tryInitialize wouldn't fail. I didn't have any other choices. 🤣
https://github.com/line/armeria/pull/5646/files#diff-d1e7f7b4f634f59e01f367028cc27541778f896067e3c5937ff188e4b4b8a93dR213-R214
https://github.com/line/armeria/pull/5646/files#diff-17bf92a3b095e7d29e480344e322fd2c6039d7f5864050b1d532715e871538a0R160

codecov · 2024-05-24T08:51:12Z

Codecov Report

Attention: Patch coverage is 81.41593% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 74.15%. Comparing base (14c5566) to head (e77bfa3).
Report is 14 commits behind head on main.

Files	Patch %	Lines
...orp/armeria/client/AbstractHttpRequestHandler.java	65.38%	7 Missing and 2 partials ⚠️
...p/armeria/client/AggregatedHttpRequestHandler.java	86.48%	0 Missing and 5 partials ⚠️
.../armeria/client/AbstractHttpRequestSubscriber.java	55.55%	3 Missing and 1 partial ⚠️
...m/linecorp/armeria/client/HttpResponseWrapper.java	78.57%	0 Missing and 3 partials ⚠️

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.
📢 Have feedback on the report? Share it here.

moromin added 5 commits April 22, 2024 16:55

feat: handle expect 100 header in client side

2165035

wip: adding tests with ServerSocket

7c169f4

feat: handle http1 requests

ec5b625

test: add http2 tests

ebd0033

style: fix check sytle errors

6a1eeb9

trustin reviewed Apr 30, 2024

View reviewed changes

moromin added 4 commits April 30, 2024 19:19

refactor: call handler.handle100Continue() in the wrapper instead of …

57187d6

…decoder

refactor: mark requestHandler as @nullable

9ce9e4c

test: add check to verify the data is not sent until sending 100

1df52cc

docs: mark @nullable and add comments about requestHandler in respons…

3c36242

…eWrapper

moromin requested review from ikhoon, jrhee17 and minwoox as code owners April 30, 2024 14:53

trustin reviewed May 2, 2024

View reviewed changes

refactor: wait only for check of no data sent before sending response

4c64e95

jrhee17 reviewed May 3, 2024

View reviewed changes

feat: flush channel and check subscription state when calling resume()

694a6d4

jrhee17 added the new feature label May 7, 2024

jrhee17 added this to the 1.29.0 milestone May 7, 2024

ikhoon reviewed May 7, 2024

View reviewed changes

trustin approved these changes May 7, 2024

View reviewed changes

refactor: check status in http1 success cases

e74b028

minwoox reviewed May 8, 2024

View reviewed changes

moromin added 2 commits May 10, 2024 00:56

refactor: throw exception when an empty content is set and ignore the…

b0b0dde

… header when streaming request

refactor: ignore the header when using a streaming request

3c51389

moromin added 3 commits May 15, 2024 00:17

refactor: handle request with starndard flow when other status received

781c1f5

wip: try to repeat request without the header when 417 received

4c1870b

style: fix style errors

01d7b18

feat: introduce new state and send an empty data when 417

c566cc6

minwoox reviewed May 20, 2024

View reviewed changes

Fix

9311e37

Add protocol

1d520c2

Merge branch 'main' into respect_100-continue_header_on_client

e77bfa3


		out.write("HTTP/1.1 100 Continue\r\n\r\n".getBytes(StandardCharsets.US_ASCII));

		assertThat(in.readLine()).isEqualTo("foo");

	private static boolean handleExpect100ContinueHeader(RequestHeaders headers) {
	private static boolean shouldExpect100ContinueHeader(RequestHeaders headers) {

-                    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));

Support 100-continue header on client side #5646

Are you sure you want to change the base?

Support 100-continue header on client side #5646

Conversation

moromin commented Apr 27, 2024

github-actions bot commented Apr 27, 2024 • edited

🔍 Build Scan® (commit: e77bfa3)

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

moromin commented May 8, 2024

minwoox commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 commented May 9, 2024

moromin commented May 14, 2024 • edited

AggregatedHandler vs. Subscriber

Empty Content

Response Status: 100 Continue

Response Status: 417 Expectation Failed

Response Status: Others

moromin commented May 14, 2024

minwoox commented May 16, 2024

moromin commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented May 20, 2024

moromin commented May 21, 2024

minwoox commented May 21, 2024 • edited

codecov bot commented May 24, 2024

Codecov Report

Support `100-continue` header on client side #5646

Support `100-continue` header on client side #5646

github-actions bot commented Apr 27, 2024 •

edited

🔍 Build Scan® (commit: `e77bfa3`)

trustin May 7, 2024 •

edited

minwoox May 8, 2024 •

edited

moromin commented May 14, 2024 •

edited

minwoox commented May 21, 2024 •

edited