andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

SimpleChannelPromiseAggregator use first exception instead of last (#11168)

Browse Source 
Motivation:
SimpleChannelPromiseAggregator implements the promise API and allows for
multiple operations to share a common promise. It currently propagates
the last exception to occur, but this may mask the original exception
which lead to the last exception and make debugging more difficult.

Modifications:
- SimpleChannelPromiseAggregator propagates the first exception instead
  of the last exception.

Result:
Fixes https://github.com/netty/netty/issues/11161.
This commit is contained in:
Scott Mitchell 2021-04-22 03:17:47 -07:00 committed by Norman Maurer
parent a5e3b59de3
commit 8c12ad4cee
2 changed files with 42 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
View File

@ -262,7 +262,7 @@ public final class Http2CodecUtil {
private final ChannelPromise promise;
private int expectedCount;
private int doneCount;
private Throwable lastFailure;
private Throwable aggregateFailure;
private boolean doneAllocating;
SimpleChannelPromiseAggregator(ChannelPromise promise, Channel c, EventExecutor e) {
@ -301,7 +301,7 @@ public final class Http2CodecUtil {
public boolean tryFailure(Throwable cause) {
if (allowFailure()) {
++doneCount;
lastFailure = cause;
setAggregateFailure(cause);
if (allPromisesDone()) {
return tryPromise();
}
@ -322,7 +322,7 @@ public final class Http2CodecUtil {
public ChannelPromise setFailure(Throwable cause) {
if (allowFailure()) {
++doneCount;
lastFailure = cause;
setAggregateFailure(cause);
if (allPromisesDone()) {
return setPromise();
}
@ -368,22 +368,28 @@ public final class Http2CodecUtil {
}
private ChannelPromise setPromise() {
if (lastFailure == null) {
if (aggregateFailure == null) {
promise.setSuccess();
return super.setSuccess(null);
} else {
promise.setFailure(lastFailure);
return super.setFailure(lastFailure);
promise.setFailure(aggregateFailure);
return super.setFailure(aggregateFailure);
}
}
private boolean tryPromise() {
if (lastFailure == null) {
if (aggregateFailure == null) {
promise.trySuccess();
return super.trySuccess(null);
} else {
promise.tryFailure(lastFailure);
return super.tryFailure(lastFailure);
promise.tryFailure(aggregateFailure);
return super.tryFailure(aggregateFailure);
}
}
private void setAggregateFailure(Throwable cause) {
if (aggregateFailure == null) {
aggregateFailure = cause;
}
}
}

27
codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java
View File

@ -64,8 +64,11 @@ import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
import static io.netty.handler.codec.http2.Http2TestUtil.of;
import static io.netty.util.CharsetUtil.UTF_8;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
@ -357,6 +360,30 @@ public class HttpToHttp2ConnectionHandlerTest {
assertFalse(writeFuture.isSuccess());
}
@Test
public void testInvalidStreamId() throws Exception {
bootstrapEnv(2, 1, 0);
final FullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, POST, "/foo",
Unpooled.copiedBuffer("foobar", UTF_8));
final HttpHeaders httpHeaders = request.headers();
httpHeaders.setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), -1);
httpHeaders.set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http");
httpHeaders.set(HttpHeaderNames.HOST, "localhost");
ChannelPromise writePromise = newPromise();
ChannelFuture writeFuture = clientChannel.writeAndFlush(request, writePromise);
assertTrue(writePromise.awaitUninterruptibly(WAIT_TIME_SECONDS, SECONDS));
assertTrue(writePromise.isDone());
assertFalse(writePromise.isSuccess());
Throwable cause = writePromise.cause();
assertThat(cause, instanceOf(Http2NoMoreStreamIdsException.class));
assertTrue(writeFuture.isDone());
assertFalse(writeFuture.isSuccess());
cause = writeFuture.cause();
assertThat(cause, instanceOf(Http2NoMoreStreamIdsException.class));
}
@Test
public void testRequestWithBody() throws Exception {
final String text = "foooooogoooo";