Failing test: Reentrant close in HttpRequestDecoder

[yawkat]

Discovered by fuzzing, but I'm not sure whether this can actually happen outside EmbeddedChannel.

Normally, HttpRequestDecoder will strictly follow the pattern (HttpRequest HttpContent* LastHttpContent)* for downstream handlers. However, when a downstream handler closes the channel inside a channelRead for a HttpRequest, this may reorder inputs, eg HttpRequest HttpRequest LastHttpContent LastHttpContent. This is mostly ByteToMessageDecoder's fault, and I'm not sure how to fix it in a performance-neutral way. Basically, HttpRequestDecoder.channelInactive is called while HttpRequestDecoder.channelRead is still running, so the next request gets inserted mid-request.

If this can only happen in EmbeddedChannel (which I'm not sure of), then we should just fix EmbeddedChannel instead.

[yawkat]

Hmm it seems AbstractChannel guards against exactly this:

netty/transport/src/main/java/io/netty/channel/AbstractChannel.java

Lines 795 to 826 in 3710cf1

	// As a user may call deregister() from within any method while doing processing in the ChannelPipeline,
	// we need to ensure we do the actual deregister operation later. This is needed as for example,
	// we may be in the ByteToMessageDecoder.callDecode(...) method and so still try to do processing in
	// the old EventLoop while the user already registered the Channel to a new EventLoop. Without delay,
	// the deregister operation this could lead to have a handler invoked by different EventLoop and so
	// threads.
	//
	// See:
	// https://github.com/netty/netty/issues/4435
	invokeLater(new Runnable() {
	@Override
	public void run() {
	try {
	doDeregister();
	} catch (Throwable t) {
	logger.warn("Unexpected exception occurred while deregistering a channel.", t);
	} finally {
	if (fireChannelInactive) {
	pipeline.fireChannelInactive();
	}
	// Some transports like local and AIO does not allow the deregistration of
	// an open channel. Their doDeregister() calls close(). Consequently,
	// close() calls deregister() again - no need to fire channelUnregistered, so check
	// if it was registered.
	if (registered) {
	registered = false;
	pipeline.fireChannelUnregistered();
	}
	safeSetSuccess(promise);
	}
	}
	});

However EmbeddedChannel.close runs pending tasks, even if we are already running event loop stuff. So the invokeLater is not effective.

[normanmaurer]

@yawkat thanks for reporting... it took me a while but I just pushed a fix for this into your branch. PTAL.

[chrisvest]

The FlushConsolidationHandlerTest is consistently failing.

[yawkat]

a bit of a semantic change, we now run tasks after more operations. but LGTM

[normanmaurer]

The FlushConsolidationHandlerTest is consistently failing.

Will check

[normanmaurer]

damn it... totally forgot about this one. Will check tomorrow

[normanmaurer]

@yawkat @chrisvest I think this should be ready to review now

[yawkat]

LGTM