Expose unexpected decoding exception reason - #5620
Conversation
…tion when encounter edge case exception
|
|
🔍 Build Scan® (commit: e3da519)
|
| import io.netty.channel.embedded.EmbeddedChannel; | ||
| import io.netty.handler.codec.compression.DecompressionException; | ||
|
|
||
| class TestStreamDecoder implements StreamDecoder { |
There was a problem hiding this comment.
What do you think of extending AbstractStreamDecoder instead of copying the code?
If necessary, we may move this class to common.encoding.
There was a problem hiding this comment.
That would be great. I've changed this class' directory path. Extending AbstractStreamDecoder for the test class is what i wanted, not recreating the same code. Thanks for allowed to do this.
| throw new UnexpectedDecodeException( | ||
| "Unexpected exception has occurred. " + | ||
| "This exception is not caused by the buffer reaching max size. Exception message: " + | ||
| message); |
There was a problem hiding this comment.
I think it is okay to re-throw the exception as is if we don't have additional information for the error message.
That said, your test code is invaluable to verify the code.
There was a problem hiding this comment.
Your words mean a lot to me. Thank you.
|
And please sign the ICLA 🙇 |
…change the name of test classes
|
I've signed ICLA. Sorry for missed this. |
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Changes look good to me once https://github.com/line/armeria/pull/5620/files#r1574617073 is handled 👍 👍 👍
|
I've applied trustin's review. |
trustin
left a comment
trustin
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. Just one last thing 😅
| final ByteBuf mockByteBuf = mock(ByteBuf.class); | ||
| final HttpData mockData = mock(HttpData.class); | ||
| when(mockData.byteBuf()).thenReturn(mockByteBuf); | ||
|
|
||
| final EmbeddedChannel channel = new AlwaysFailureEmbeddedChannel(false); | ||
| final StreamDecoder decoder = new TestStreamDecoder(channel, new SnappyFrameDecoder(), | ||
| mock(ByteBufAllocator.class), 100); |
There was a problem hiding this comment.
Use a mock only when we really need it?
There was a problem hiding this comment.
Note: ByteBufAllocator doesn't need to be a mock.
|
Revised and cleaned up the PR description and commit message, which was previously out of sync. |
6 participants
Related issue: #5177
Motivation:
AbstractStreamDecoder#decodesilently ignores the failure to decode messages.We probably want to let the caller know a decoding failure has occurred,
instead of silently ignoring and returning an empty result.
Modifications:
AbstractStreamDecoderto rethrow the caught exception for all cases.Result:
AbstractStreamDecoder#5177