Support `100-continue` header on client side

[moromin]

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 #5394

[github-actions[bot]]

🔍 Build Scan® (commit: e3e5f57d79ce9da31a8446f4bb870e465bc41921)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/jjp4n6u6cvo5o
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/x6fi2ynovctni
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/yvuvgts4dcowm
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/x5qdsb4u7sdbi
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/qz2754r2mkoo4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/zdxq3n5o2qz52
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/fu6hgwpbjkahm
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/vr4x7kzinxmcg

[moromin]

As with the other cases, we tried to future.join() and verify status for the HTTP1 failure case: https://github.com/line/armeria/blob/e74b028b5e6fcd608e8a5c6c87be59443b765a71/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java#L122 https://github.com/line/armeria/blob/e74b028b5e6fcd608e8a5c6c87be59443b765a71/core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java#L315

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

[minwoox]

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.

[jrhee17]

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

https://github.com/line/armeria/pull/5646#discussion_r1588860191 Like pointed out in this comment, the readLine() is expecting a newline (e.g. foo\n)

[moromin]

🔵 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]

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. https://github.com/line/armeria/blob/01d7b18a5262ad2864abaab5bccebad02192da09/core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java#L127

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

[minwoox]

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: https://github.com/line/armeria/blob/01d7b18a5262ad2864abaab5bccebad02192da09/core/src/main/java/com/linecorp/armeria/client/AggregatedHttpRequestHandler.java#L130
• 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; 
  

https://github.com/line/armeria/blob/4c1870b0ece98154708ba1b4fda72afc71a87326/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java#L208

• Could you also fix Http2ResponseDecoder? https://github.com/line/armeria/blob/57187d67ce0b30dac50b90aa82a987cddd06a3d0/core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java#L212
• 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]

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]

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

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]

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[bot]]

Codecov Report

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

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

:exclamation: Current head e77bfa3 differs from pull request most recent head e3e5f57

Please upload reports for the commit e3e5f57 to get more accurate results.

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

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[minwoox]

@moromin Had a chat with the team and we've decided not to implement repeat because:

• Most servers support 100-continue.
• It's much simpler to just fail the request and let the user retry.