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.

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 treat errors as decoder exception (redux) #7279

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@jasontedor
Contributor

jasontedor commented Oct 5, 2017

Motivation: Today when Netty encounters a general error while decoding it treats this as a decoder exception. However, for fatal causes this should not be treated as such, instead the fatal error should be carried up the stack without the callee having to unwind causes. This was probably done for byte to byte message decoder but is now done for all decoders.

Modifications: Instead of translating any error to a decoder exception, we let those unwind out the stack (note that finally blocks still execute) except in places where an event needs to fire where we fire with the error instead of wrapping in a decoder exception.

Result: Fatal errors will not be treated as innocent decoder exceptions.

Relates #6096

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Oct 5, 2017

Contributor

This is a follow up to #7276.

Contributor

jasontedor commented Oct 5, 2017

This is a follow up to #7276.

@normanmaurer

one change then LGTM

@@ -418,7 +418,7 @@ protected void callDecode(ChannelHandlerContext ctx, ByteBuf in, List<Object> ou
}
} catch (DecoderException e) {
throw e;
} catch (Throwable cause) {
} catch (Exception cause) {

This comment has been minimized.

@johnou

johnou Oct 6, 2017

Contributor

based on the javadoc of ReplayingDecoder I think we might need special handling here.. https://github.com/jasontedor/netty/blob/8ad05f00703ed42480e4b83c5d550dce68259b60/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java#L77

* {@link ReplayingDecoder} passes a specialized {@link ByteBuf}
 * implementation which throws an {@link Error} of certain type when there's not
 * enough data in the buffer.
@johnou

johnou Oct 6, 2017

Contributor

based on the javadoc of ReplayingDecoder I think we might need special handling here.. https://github.com/jasontedor/netty/blob/8ad05f00703ed42480e4b83c5d550dce68259b60/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java#L77

* {@link ReplayingDecoder} passes a specialized {@link ByteBuf}
 * implementation which throws an {@link Error} of certain type when there's not
 * enough data in the buffer.
@johnou

johnou Oct 6, 2017

Contributor

This comment has been minimized.

@johnou

johnou Oct 6, 2017

Contributor

actually it's caught further up, might be okay.

@johnou

johnou Oct 6, 2017

Contributor

actually it's caught further up, might be okay.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 6, 2017

Member

@johnou yep I think its fine

@normanmaurer

normanmaurer Oct 6, 2017

Member

@johnou yep I think its fine

@johnou

johnou approved these changes Oct 6, 2017

Do not treat errors as decoder exception (redux)
Motivation: Today when Netty encounters a general error while decoding
it treats this as a decoder exception. However, for fatal causes this
should not be treated as such, instead the fatal error should be carried
up the stack without the callee having to unwind causes. This was
probably done for byte to byte message decoder but is now done for all
decoders.

Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute) except in places where an event needs to fire where we fire
with the error instead of wrapping in a decoder exception.

Result: Fatal errors will not be treated as innocent decoder exceptions.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 7, 2017

Member

Cherry-picked into 4.1 (3fe1f71) and 4.0 (b5b53da)

Member

normanmaurer commented Oct 7, 2017

Cherry-picked into 4.1 (3fe1f71) and 4.0 (b5b53da)

@normanmaurer normanmaurer added this to the 4.0.53.Final milestone Oct 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment