-
Notifications
You must be signed in to change notification settings - Fork 896
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix a bug where
ClosedSessionException is set to responseCause fo…
…r a success response. (#4632) Motivation: It would be legitimate that an HTTP/1 client closes a connection after data is fully received. Although the connection is closed, the stream on the server side can be still open. Because `HttpResponseSubscriber` may be complete after fully sending data and then receiving `onComplete()` signal. Receiving `onComplete()`, `HttpResponseSubscriber` tries to write an empty chunk as a mark of EOS to the disconnected channel. https://github.com/line/armeria/blob/fee87f8da942d0e6343e814489524b73db60ef3b/core/src/main/java/com/linecorp/armeria/server/HttpResponseSubscriber.java#L317-L320 The write attempt would fail with `ClosedSessionException` and the failure is set to `responseCause`. Practically, `responseCause` is meaningless and false positive. Modifications: - Check a connection is active as well when `Http1ObjectEncoder` determines a session is writable. - Do not send EOS and mark a request as success when an HTTP/1 session is inactive when handling `onComplete()` in `HttpResponseSubscriber` Result: You no longer see `ClosedSessionException` when a connection is closed after a response data has been fully sent in HTTP/1.
- Loading branch information
Showing
3 changed files
with
156 additions
and
16 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
120 changes: 120 additions & 0 deletions
120
core/src/test/java/com/linecorp/armeria/server/Http1ServerEarlyDisconnectionTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /* | ||
| * Copyright 2023 LINE Corporation | ||
| * | ||
| * LINE Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package com.linecorp.armeria.server; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.util.concurrent.CountDownLatch; | ||
|
|
||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.RegisterExtension; | ||
| import org.reactivestreams.Subscriber; | ||
| import org.reactivestreams.Subscription; | ||
|
|
||
| import com.linecorp.armeria.client.ClientFactory; | ||
| import com.linecorp.armeria.client.WebClient; | ||
| import com.linecorp.armeria.common.HttpData; | ||
| import com.linecorp.armeria.common.HttpResponse; | ||
| import com.linecorp.armeria.common.HttpResponseWriter; | ||
| import com.linecorp.armeria.common.ResponseHeaders; | ||
| import com.linecorp.armeria.common.SessionProtocol; | ||
| import com.linecorp.armeria.common.SplitHttpResponse; | ||
| import com.linecorp.armeria.common.logging.RequestLog; | ||
| import com.linecorp.armeria.internal.testing.FlakyTest; | ||
| import com.linecorp.armeria.testing.junit5.server.ServerExtension; | ||
|
|
||
| @FlakyTest | ||
| class Http1ServerEarlyDisconnectionTest { | ||
|
|
||
| @RegisterExtension | ||
| static ServerExtension server = new ServerExtension() { | ||
| @Override | ||
| protected void configure(ServerBuilder sb) { | ||
| sb.service("/", (ctx, req) -> { | ||
| final HttpResponseWriter writer = HttpResponse.streaming(); | ||
| writer.write(ResponseHeaders.builder(200) | ||
| .contentLength(10) | ||
| .build()); | ||
| ctx.blockingTaskExecutor().execute(() -> { | ||
| writer.write(HttpData.ofUtf8("0123456789")); | ||
|
|
||
| // Wait for the client to close the connection. | ||
| // Note: The sleep duration should be less than 1 second after which `ServerHandler` | ||
| // calls `cleanup()` to remove `unfinishedRequests`. | ||
| try { | ||
| latch.await(); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| writer.close(); | ||
| }); | ||
|
|
||
| return writer; | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| private static CountDownLatch latch; | ||
|
|
||
| @BeforeAll | ||
| static void beforeAll() { | ||
| latch = new CountDownLatch(1); | ||
| } | ||
|
|
||
| @Test | ||
| void closeConnectionWhenAllContentAreReceived() throws InterruptedException { | ||
| final ClientFactory clientFactory = ClientFactory.builder().build(); | ||
| final WebClient client = WebClient.builder(server.uri(SessionProtocol.H1C)) | ||
| .factory(clientFactory) | ||
| .build(); | ||
| final HttpResponse response = client.get("/"); | ||
| final SplitHttpResponse split = response.split(); | ||
| final ResponseHeaders headers = split.headers().join(); | ||
| final long contentLength = headers.contentLength(); | ||
| split.body().subscribe(new Subscriber<HttpData>() { | ||
|
|
||
| private int received; | ||
|
|
||
| @Override | ||
| public void onSubscribe(Subscription s) { | ||
| s.request(Long.MAX_VALUE); | ||
| } | ||
|
|
||
| @Override | ||
| public void onNext(HttpData httpData) { | ||
| received += httpData.length(); | ||
| if (received >= contentLength) { | ||
| // All data is received, so it should be safe to close the connection. | ||
| clientFactory.close(); | ||
| latch.countDown(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(Throwable t) {} | ||
|
|
||
| @Override | ||
| public void onComplete() {} | ||
| }); | ||
|
|
||
| final ServiceRequestContext ctx = server.requestContextCaptor().take(); | ||
| final RequestLog log = ctx.log().whenComplete().join(); | ||
| assertThat(log.responseCause()).isNull(); | ||
| } | ||
| } |