Close the connection when decoding exception happens

[violetagg]

Fixes #3285

[pderop]

it seems some other tests are not passing anymore, like HttpServerTest.keepAlive()

[violetagg]

it seems some other tests are not passing anymore, like HttpServerTest.keepAlive()

yeah the change should be in the if (decoderResult.isFailure()) {

[pderop]

I see that the ctx is closed on decoding failure, but only if the logger is enabled in DEBUG mode ?

I mean, instead of

				if (log.isDebugEnabled()) {
					if (msg instanceof DecoderResultProvider) {
						DecoderResult decoderResult = ((DecoderResultProvider) msg).decoderResult();
						if (decoderResult.isFailure()) {
							log.debug(format(ctx.channel(), "Decoding failed."), decoderResult.cause());
							//"FutureReturnValueIgnored" this is deliberate
							ctx.close();
							listener.onUncaughtException(connection, decoderResult.cause());
						}
					}

					log.debug(format(ctx.channel(), "No ChannelOperation attached."));
				}

shouldn't it be something like this ?

				if (msg instanceof DecoderResultProvider) {
					DecoderResult decoderResult = ((DecoderResultProvider) msg).decoderResult();
					if (decoderResult.isFailure()) {
						if (log.isDebugEnabled()) {
							log.debug(format(ctx.channel(), "Decoding failed."), decoderResult.cause());
						}
						//"FutureReturnValueIgnored" this is deliberate
						ctx.close();
						listener.onUncaughtException(connection, decoderResult.cause());
					}

					if (log.isDebugEnabled()) {
						log.debug(format(ctx.channel(), "No ChannelOperation attached."));
					}
				}

[violetagg]

@pderop Thanks, check the new commit

[pderop]

LGTM 🚀

[violetagg]

@pderop I found one additional issue, commit is commit in the next minutes.

[violetagg]

I will move the fix for the next release. I want to play more with various scenarios.

[pderop]

ok Violeta.

[violetagg]

@pderop The PR is ready for review

[violetagg]

@pderop Thanks!