SimpleChannelPromiseAggregator use first exception instead of last (#11168)

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 GitHub
parent acd9b383bc
commit 75c1134c0d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 42 additions and 9 deletions

View File

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

View File

@ -63,8 +63,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.handler.codec.http2.Http2TestUtil.of;
import static io.netty.util.CharsetUtil.UTF_8; import static io.netty.util.CharsetUtil.UTF_8;
import static java.util.concurrent.TimeUnit.SECONDS; 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.assertEquals;
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.any; import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.anyBoolean;
@ -356,6 +359,30 @@ public class HttpToHttp2ConnectionHandlerTest {
assertFalse(writeFuture.isSuccess()); 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 @Test
public void testRequestWithBody() throws Exception { public void testRequestWithBody() throws Exception {
final String text = "foooooogoooo"; final String text = "foooooogoooo";