Skip to content
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.

Sign up for GitHub

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

Do not send RST_STREAM when UnprocessedRequestException is raised #6157

Merged
merged 5 commits into from
Mar 12, 2025
Merged

Do not send RST_STREAM when UnprocessedRequestException is raised #6157

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
88dc159
Do not send RST_STREAM when UnprocessedRequestException is raised
minwoox Mar 12, 2025
5d79768
copyright
minwoox Mar 12, 2025
fb5d7c8
more fix
minwoox Mar 12, 2025
9cf0e07
lint
minwoox Mar 12, 2025
c1a3632
Address the comment from @ikhoon
minwoox Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ private void onWrapperCompleted(HttpResponseWrapper resWrapper, int id, @Nullabl
resWrapper.onSubscriptionCancelled(cause);

if (cause != null) {
if (cause instanceof UnprocessedRequestException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not need to send RST_STREAM when the stream was closed or a RST_STREAM was received before. I was wondering if our logic already handles the cases.

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

Lines 138 to 150 in e08527d

if (res.isOpen()) {
if (!goAwayHandler.receivedGoAway()) {
res.close(newClosedStreamException(channel()));
return;
}
final int lastStreamId = conn.local().lastStreamKnownByPeer();
if (stream.id() > lastStreamId) {
res.close(UnprocessedRequestException.of(GoAwayReceivedException.get()));
} else {
res.close(newClosedStreamException(channel()));
}
}

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

Line 314 in e08527d

res.close(cause);

Copy link
Contributor Author

@minwoox minwoox Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Netty handles those cases:
https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L808-L810
The problem that I fixed is that it tries to send RST_STREAM even though the stream was removed. Let me also add that condition to prevent the situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Netty seems to send another RST_STREAM by encoder.writeRstStream() when the client receives an RST_STREAM or the connection is closed.

My suggestion is:

if (cause instanceof UnprocessedRequestException || 
    cause instanceof ClosedStreamException) {
   return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks addressed. 👍

return;
}
// Removing the response and decrementing `unfinishedResponses` isn't done immediately
// here. Instead, we rely on `Http2ResponseDecoder#onStreamClosed` to decrement
// `unfinishedResponses` after Netty decrements `numActiveStreams` in `DefaultHttp2Connection`
Expand Down
65 changes: 65 additions & 0 deletions core/src/test/java/com/linecorp/armeria/client/HeaderListSizeExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright 2025 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.client;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.time.Duration;
import java.util.concurrent.CompletableFuture;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.google.common.base.Strings;

import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpMethod;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.armeria.server.ServerBuilder;
import com.linecorp.armeria.testing.junit5.server.ServerExtension;

import io.netty.handler.codec.http2.Http2Exception.HeaderListSizeException;

class HeaderListSizeExceptionTest {

@RegisterExtension
static ServerExtension server = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
sb.service("/", (ctx, req) -> HttpResponse.delayed(
() -> HttpResponse.of("OK"), Duration.ofMillis(100)));
}
};


@Test
void doNotSendRstStreamWhenHeaderListSizeExceptionIsRaised() throws InterruptedException {
final CompletableFuture<AggregatedHttpResponse> future = server.webClient().get("/").aggregate();
final String a = Strings.repeat("aa", 10000);
final RequestHeaders headers = RequestHeaders.of(HttpMethod.GET, "/", "foo", "bar",
"baz", a);
assertThatThrownBy(() -> server.webClient().execute(headers).aggregate().join())
.hasCauseInstanceOf(UnprocessedRequestException.class)
.cause()
.hasCauseInstanceOf(HeaderListSizeException.class);
// If the client sends RST_STREAM with invalid stream ID, the server will send GOAWAY back thus
// the first request will be failed with ClosedSessionException.
assertThat(future.join().status()).isSameAs(HttpStatus.OK);
}
}
Loading