Skip to content

Commit 1df5b02

Browse files
normanmaurerScottmitch
authored andcommittedJan 25, 2018
Do not fire outbound exception throught the pipeline when using Http2FrameCodec / Http2MultiplexCodec
Motivation: Usually when using netty exceptions which happen for outbound operations should not be fired through the pipeline but only the ChannelPromise should be failed. Modifications: - Change Http2LifecycleManager.onError(...) to take also an boolean that indicate if the error was caused by an outbound operation - Channel Http2ConnectionHandler.on*Error(...) methods to also take this boolean - Change Http2FrameCodec to only fire exceptions through the pipeline if these are not outbound operations related - Add unit test. Result: More consistent error handling when using Http2FrameCodec and Http2MultiplexCodec.
1 parent 8a095d0 commit 1df5b02

File tree

6 files changed

+74
-36
lines changed

6 files changed

+74
-36
lines changed
 

‎codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java

+7-7
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
211211
// other headers containing pseudo-header fields.
212212
stream.headersSent(isInformational);
213213
} else {
214-
lifecycleManager.onError(ctx, failureCause);
214+
lifecycleManager.onError(ctx, true, failureCause);
215215
}
216216

217217
return future;
@@ -223,7 +223,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
223223
return promise;
224224
}
225225
} catch (Throwable t) {
226-
lifecycleManager.onError(ctx, t);
226+
lifecycleManager.onError(ctx, true, t);
227227
promise.tryFailure(t);
228228
return promise;
229229
}
@@ -287,11 +287,11 @@ public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId, i
287287
if (failureCause == null) {
288288
stream.pushPromiseSent();
289289
} else {
290-
lifecycleManager.onError(ctx, failureCause);
290+
lifecycleManager.onError(ctx, true, failureCause);
291291
}
292292
return future;
293293
} catch (Throwable t) {
294-
lifecycleManager.onError(ctx, t);
294+
lifecycleManager.onError(ctx, true, t);
295295
promise.tryFailure(t);
296296
return promise;
297297
}
@@ -376,7 +376,7 @@ public void error(ChannelHandlerContext ctx, Throwable cause) {
376376
queue.releaseAndFailAll(cause);
377377
// Don't update dataSize because we need to ensure the size() method returns a consistent size even after
378378
// error so we don't invalidate flow control when returning bytes to flow control.
379-
lifecycleManager.onError(ctx, cause);
379+
lifecycleManager.onError(ctx, true, cause);
380380
}
381381

382382
@Override
@@ -456,7 +456,7 @@ public int size() {
456456
@Override
457457
public void error(ChannelHandlerContext ctx, Throwable cause) {
458458
if (ctx != null) {
459-
lifecycleManager.onError(ctx, cause);
459+
lifecycleManager.onError(ctx, true, cause);
460460
}
461461
promise.tryFailure(cause);
462462
}
@@ -476,7 +476,7 @@ public void write(ChannelHandlerContext ctx, int allowedBytes) {
476476
if (failureCause == null) {
477477
stream.headersSent(isInformational);
478478
} else {
479-
lifecycleManager.onError(ctx, failureCause);
479+
lifecycleManager.onError(ctx, true, failureCause);
480480
}
481481
}
482482

‎codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

+18-15
Original file line numberDiff line numberDiff line change
@@ -200,9 +200,9 @@ public void flush(ChannelHandlerContext ctx) {
200200
encoder.flowController().writePendingBytes();
201201
ctx.flush();
202202
} catch (Http2Exception e) {
203-
onError(ctx, e);
203+
onError(ctx, true, e);
204204
} catch (Throwable cause) {
205-
onError(ctx, connectionError(INTERNAL_ERROR, cause, "Error flushing"));
205+
onError(ctx, true, connectionError(INTERNAL_ERROR, cause, "Error flushing"));
206206
}
207207
}
208208

@@ -254,7 +254,7 @@ public void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) thro
254254
byteDecoder.decode(ctx, in, out);
255255
}
256256
} catch (Throwable e) {
257-
onError(ctx, e);
257+
onError(ctx, false, e);
258258
}
259259
}
260260

@@ -389,7 +389,7 @@ public void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) thro
389389
try {
390390
decoder.decodeFrame(ctx, in, out);
391391
} catch (Throwable e) {
392-
onError(ctx, e);
392+
onError(ctx, false, e);
393393
}
394394
}
395395
}
@@ -538,7 +538,7 @@ void channelReadComplete0(ChannelHandlerContext ctx) throws Exception {
538538
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
539539
if (getEmbeddedHttp2Exception(cause) != null) {
540540
// Some exception in the causality chain is an Http2Exception - handle it.
541-
onError(ctx, cause);
541+
onError(ctx, false, cause);
542542
} else {
543543
super.exceptionCaught(ctx, cause);
544544
}
@@ -604,17 +604,17 @@ public void operationComplete(ChannelFuture future) throws Exception {
604604
* Central handler for all exceptions caught during HTTP/2 processing.
605605
*/
606606
@Override
607-
public void onError(ChannelHandlerContext ctx, Throwable cause) {
607+
public void onError(ChannelHandlerContext ctx, boolean outbound, Throwable cause) {
608608
Http2Exception embedded = getEmbeddedHttp2Exception(cause);
609609
if (isStreamError(embedded)) {
610-
onStreamError(ctx, cause, (StreamException) embedded);
610+
onStreamError(ctx, outbound, cause, (StreamException) embedded);
611611
} else if (embedded instanceof CompositeStreamException) {
612612
CompositeStreamException compositException = (CompositeStreamException) embedded;
613613
for (StreamException streamException : compositException) {
614-
onStreamError(ctx, cause, streamException);
614+
onStreamError(ctx, outbound, cause, streamException);
615615
}
616616
} else {
617-
onConnectionError(ctx, cause, embedded);
617+
onConnectionError(ctx, outbound, cause, embedded);
618618
}
619619
ctx.flush();
620620
}
@@ -633,11 +633,13 @@ protected boolean isGracefulShutdownComplete() {
633633
* streams are closed, the connection is shut down.
634634
*
635635
* @param ctx the channel context
636+
* @param outbound {@code true} if the error was caused by an outbound operation.
636637
* @param cause the exception that was caught
637638
* @param http2Ex the {@link Http2Exception} that is embedded in the causality chain. This may
638639
* be {@code null} if it's an unknown exception.
639640
*/
640-
protected void onConnectionError(ChannelHandlerContext ctx, Throwable cause, Http2Exception http2Ex) {
641+
protected void onConnectionError(ChannelHandlerContext ctx, boolean outbound,
642+
Throwable cause, Http2Exception http2Ex) {
641643
if (http2Ex == null) {
642644
http2Ex = new Http2Exception(INTERNAL_ERROR, cause.getMessage(), cause);
643645
}
@@ -659,11 +661,12 @@ protected void onConnectionError(ChannelHandlerContext ctx, Throwable cause, Htt
659661
* stream.
660662
*
661663
* @param ctx the channel context
664+
* @param outbound {@code true} if the error was caused by an outbound operation.
662665
* @param cause the exception that was caught
663666
* @param http2Ex the {@link StreamException} that is embedded in the causality chain.
664667
*/
665-
protected void onStreamError(ChannelHandlerContext ctx, @SuppressWarnings("unused") Throwable cause,
666-
StreamException http2Ex) {
668+
protected void onStreamError(ChannelHandlerContext ctx, boolean outbound,
669+
@SuppressWarnings("unused") Throwable cause, StreamException http2Ex) {
667670
final int streamId = http2Ex.streamId();
668671
Http2Stream stream = connection().stream(streamId);
669672

@@ -692,7 +695,7 @@ protected void onStreamError(ChannelHandlerContext ctx, @SuppressWarnings("unuse
692695
try {
693696
handleServerHeaderDecodeSizeError(ctx, stream);
694697
} catch (Throwable cause2) {
695-
onError(ctx, connectionError(INTERNAL_ERROR, cause2, "Error DecodeSizeError"));
698+
onError(ctx, outbound, connectionError(INTERNAL_ERROR, cause2, "Error DecodeSizeError"));
696699
}
697700
}
698701
}
@@ -867,13 +870,13 @@ private void processRstStreamWriteResult(ChannelHandlerContext ctx, Http2Stream
867870
closeStream(stream, future);
868871
} else {
869872
// The connection will be closed and so no need to change the resetSent flag to false.
870-
onConnectionError(ctx, future.cause(), null);
873+
onConnectionError(ctx, true, future.cause(), null);
871874
}
872875
}
873876

874877
private void closeConnectionOnError(ChannelHandlerContext ctx, ChannelFuture future) {
875878
if (!future.isSuccess()) {
876-
onConnectionError(ctx, future.cause(), null);
879+
onConnectionError(ctx, true, future.cause(), null);
877880
}
878881
}
879882

‎codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java

+18-10
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ public boolean visit(Http2Stream stream) {
186186
try {
187187
return streamVisitor.visit((Http2FrameStream) stream.getProperty(streamKey));
188188
} catch (Throwable cause) {
189-
onError(ctx, cause);
189+
onError(ctx, false, cause);
190190
return false;
191191
}
192192
}
@@ -431,38 +431,46 @@ public void onStreamHalfClosed(Http2Stream stream) {
431431
}
432432

433433
@Override
434-
protected void onConnectionError(ChannelHandlerContext ctx, Throwable cause, Http2Exception http2Ex) {
435-
// allow the user to handle it first in the pipeline, and then automatically clean up.
436-
// If this is not desired behavior the user can override this method.
437-
ctx.fireExceptionCaught(cause);
438-
super.onConnectionError(ctx, cause, http2Ex);
434+
protected void onConnectionError(
435+
ChannelHandlerContext ctx, boolean outbound, Throwable cause, Http2Exception http2Ex) {
436+
if (!outbound) {
437+
// allow the user to handle it first in the pipeline, and then automatically clean up.
438+
// If this is not desired behavior the user can override this method.
439+
//
440+
// We only forward non outbound errors as outbound errors will already be reflected by failing the promise.
441+
ctx.fireExceptionCaught(cause);
442+
}
443+
super.onConnectionError(ctx, outbound, cause, http2Ex);
439444
}
440445

441446
/**
442447
* Exceptions for unknown streams, that is streams that have no {@link Http2FrameStream} object attached
443448
* are simply logged and replied to by sending a RST_STREAM frame.
444449
*/
445450
@Override
446-
protected final void onStreamError(ChannelHandlerContext ctx, Throwable cause,
451+
protected final void onStreamError(ChannelHandlerContext ctx, boolean outbound, Throwable cause,
447452
Http2Exception.StreamException streamException) {
448453
int streamId = streamException.streamId();
449454
Http2Stream connectionStream = connection().stream(streamId);
450455
if (connectionStream == null) {
451456
onHttp2UnknownStreamError(ctx, cause, streamException);
452457
// Write a RST_STREAM
453-
super.onStreamError(ctx, cause, streamException);
458+
super.onStreamError(ctx, outbound, cause, streamException);
454459
return;
455460
}
456461

457462
Http2FrameStream stream = connectionStream.getProperty(streamKey);
458463
if (stream == null) {
459464
LOG.warn("Stream exception thrown without stream object attached.", cause);
460465
// Write a RST_STREAM
461-
super.onStreamError(ctx, cause, streamException);
466+
super.onStreamError(ctx, outbound, cause, streamException);
462467
return;
463468
}
464469

465-
onHttp2FrameStreamException(ctx, new Http2FrameStreamException(stream, streamException.error(), cause));
470+
if (!outbound) {
471+
// We only forward non outbound errors as outbound errors will already be reflected by failing the promise.
472+
onHttp2FrameStreamException(ctx, new Http2FrameStreamException(stream, streamException.error(), cause));
473+
}
466474
}
467475

468476
void onHttp2UnknownStreamError(@SuppressWarnings("unused") ChannelHandlerContext ctx, Throwable cause,

‎codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LifecycleManager.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,11 @@ ChannelFuture goAway(ChannelHandlerContext ctx, int lastStreamId, long errorCode
8888

8989
/**
9090
* Processes the given error.
91+
*
92+
* @param ctx The context used for communication and buffer allocation if necessary.
93+
* @param outbound {@code true} if the error was caused by an outbound operation and so the corresponding
94+
* {@link ChannelPromise} was failed as well.
95+
* @param cause the error.
9196
*/
92-
void onError(ChannelHandlerContext ctx, Throwable cause);
97+
void onError(ChannelHandlerContext ctx, boolean outbound, Throwable cause);
9398
}

‎codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
112112
}
113113
}
114114
} catch (Throwable t) {
115-
onError(ctx, t);
115+
onError(ctx, true, t);
116116
promiseAggregator.setFailure(t);
117117
} finally {
118118
if (release) {

‎codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java

+24-2
Original file line numberDiff line numberDiff line change
@@ -389,14 +389,14 @@ public void goAwayLastStreamIdOverflowed() throws Exception {
389389
}
390390

391391
@Test
392-
public void streamErrorShouldFireException() throws Exception {
392+
public void streamErrorShouldFireExceptionForInbound() throws Exception {
393393
frameListener.onHeadersRead(http2HandlerCtx, 3, request, 31, false);
394394

395395
Http2Stream stream = frameCodec.connection().stream(3);
396396
assertNotNull(stream);
397397

398398
StreamException streamEx = new StreamException(3, Http2Error.INTERNAL_ERROR, "foo");
399-
frameCodec.onError(http2HandlerCtx, streamEx);
399+
frameCodec.onError(http2HandlerCtx, false, streamEx);
400400

401401
Http2FrameStreamEvent event = inboundHandler.readInboundMessageOrUserEvent();
402402
assertEquals(Http2FrameStreamEvent.Type.State, event.type());
@@ -414,6 +414,28 @@ public void streamErrorShouldFireException() throws Exception {
414414
assertNull(inboundHandler.readInboundMessageOrUserEvent());
415415
}
416416

417+
@Test
418+
public void streamErrorShouldNotFireExceptionForOutbound() throws Exception {
419+
frameListener.onHeadersRead(http2HandlerCtx, 3, request, 31, false);
420+
421+
Http2Stream stream = frameCodec.connection().stream(3);
422+
assertNotNull(stream);
423+
424+
StreamException streamEx = new StreamException(3, Http2Error.INTERNAL_ERROR, "foo");
425+
frameCodec.onError(http2HandlerCtx, true, streamEx);
426+
427+
Http2FrameStreamEvent event = inboundHandler.readInboundMessageOrUserEvent();
428+
assertEquals(Http2FrameStreamEvent.Type.State, event.type());
429+
assertEquals(State.OPEN, event.stream().state());
430+
Http2HeadersFrame headersFrame = inboundHandler.readInboundMessageOrUserEvent();
431+
assertNotNull(headersFrame);
432+
433+
// No exception expected
434+
inboundHandler.checkException();
435+
436+
assertNull(inboundHandler.readInboundMessageOrUserEvent());
437+
}
438+
417439
@Test
418440
public void windowUpdateFrameDecrementsConsumedBytes() throws Exception {
419441
frameListener.onHeadersRead(http2HandlerCtx, 3, request, 31, false);

0 commit comments

Comments
 (0)
Please sign in to comment.