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 #7276

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@jasontedor
Contributor

jasontedor commented Oct 4, 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.

Modifications: Instead of translating any error to a decoder exception, we let those unwind out the stack (note that finally blocks still execute).

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

Relates #6096

Do not treat errors as decoder exception
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.

Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute).

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

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 4, 2017

Member

@jasontedor I like the change and imho its a good start in the right direction. Did you also check all the other decoder / encoders to ensure the same is done there ?

Member

normanmaurer commented Oct 4, 2017

@jasontedor I like the change and imho its a good start in the right direction. Did you also check all the other decoder / encoders to ensure the same is done there ?

} catch (Throwable t) {
throw new DecoderException(t);
} catch (Exception e) {
throw new DecoderException(e);

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Oct 4, 2017

Member

optional: maybe don't wrap RuntimeExceptions either?

@carl-mastrangelo

carl-mastrangelo Oct 4, 2017

Member

optional: maybe don't wrap RuntimeExceptions either?

This comment has been minimized.

@normanmaurer

normanmaurer Oct 4, 2017

Member

I think we should just handle Error special here imho

@normanmaurer

normanmaurer Oct 4, 2017

Member

I think we should just handle Error special here imho

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Oct 4, 2017

Contributor

Did you also check all the other decoder / encoders to ensure the same is done there ?

No, this specific one is hurting us right now as some of our users are seeing decoder exceptions hiding out of memory errors and I want to get a fix in quickly. I can take a more careful look at the others soon.

Contributor

jasontedor commented Oct 4, 2017

Did you also check all the other decoder / encoders to ensure the same is done there ?

No, this specific one is hurting us right now as some of our users are seeing decoder exceptions hiding out of memory errors and I want to get a fix in quickly. I can take a more careful look at the others soon.

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Oct 5, 2017

Contributor

@normanmaurer I've now checked all the other places and I have another patch ready to push. My preference would be to get this PR in (since it's ready) and then I can immediately open a follow-up. What do you think?

Contributor

jasontedor commented Oct 5, 2017

@normanmaurer I've now checked all the other places and I have another patch ready to push. My preference would be to get this PR in (since it's ready) and then I can immediately open a follow-up. What do you think?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 5, 2017

Member

@jasontedor works for me...

Member

normanmaurer commented Oct 5, 2017

@jasontedor works for me...

@normanmaurer normanmaurer self-assigned this Oct 5, 2017

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

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 5, 2017

Member

Cherry-picked into 4.1 (5eca326) and 4.0 (4ad6ed1) . @jasontedor thanks

Member

normanmaurer commented Oct 5, 2017

Cherry-picked into 4.1 (5eca326) and 4.0 (4ad6ed1) . @jasontedor thanks

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