HttpToHttp2ConnectionHandler#write continues on error and doesn't report correct exception

[slandelle]

Expected behavior

HttpToHttp2ConnectionHandler#write should interrupt as soon as an Exception occurs and should report it.

This looks similar to what was reported in #6705.

Actual behavior

When an Exception occurs, it's recorded in a SimpleChannelPromiseAggregator.
The execution continues, eg if the Exception occurred in writeHeaders, writeData will be performed, typically resulting in cascading failures such as "stream is missing".

SimpleChannelPromiseAggregator only reports the last Exception, losing the actual and useful one.

Steps to reproduce

Try to write a FullHttpRequest with a stream id that exceeds the max number of concurrent stream.

• DefaultHttpConnection#checkNewStreamAllowed will throw an Http2Exception("Maximum active streams violated for this endpoint.")
• DefaultHttp2ConnectionEncoder#writeHeaders0 will catch it and fail the promise
• HttpToHttp2ConnectionHandler will proceed with writeData
• DefaultHttp2ConnectionEncoder#writeData will crash in requireStream with an IllegalArgumentException("Stream no longer exists"), catch it and fail the promise, replacing the original Exception

Minimal yet complete reproducer code (or URL to code)

Netty version

4.1.63

JVM version (e.g. java -version)

irrelevant

OS version (e.g. uname -a)

irrelevant

[slandelle]

@Scottmitch May I ask some guidance regarding this issue, please?

In 6a2425b#diff-7658fbccab9a300ae85f5d2f17d9469419e4af5d04c9ba12058f1fc418ff99b5, you introduced this behavior Http2Codec.SimpleChannelPromiseAggregator to report the latest Exception instead of interrupting on the first one.

This leads here to cascading failures while interrupting is the desired behavior and to the wrong Exception being reported.

[Scottmitch]

The SimpleChannelPromiseAggregator is designed with two primary constraints in mind:

1. In general when you pass a promise to a write method the ownership (and responsibility for completion) is assumed to be transferred. SimpleChannelPromiseAggregator ensures its external API doesn't prematurely indicate "completion" to support this expected behavior and defers "completion" until:

• no more "new" operations which require "new" promises will happen (e.g. doneAllocatingPromises is called)
• as many times as "newPromise()" has been invoked operations have completed

Some areas make a best effort up-front check if the promise is already completed (e.g. AbstractChannelHandlerContext), but this isn't universal (e.g. AbstractCoalescingBufferQueue). IMHO adding additional best effort up-front checks are not ideal because they aren't reliable (e.g. if there are multiple areas which may complete the promise, you may not catch it up-front. also it is unlikely to catch every API that accepts a promise) and complicates code for invalid usage of the APIs.

2. simplify control flow and exception handling

• You don't have to wrap every call which uses the aggregate promise in a try/catch, and have conditional logic determining if you should proceed on-ward or not (e.g. just call newPromise() when you need a new promise and doneAllocatingPromises() when you are done). This may result in more intermediate exceptions but assumes exceptions are "exceptional" and favors simplified code.

I think we should preserve (1) (e.g. passing a promise to an API transfers ownership, don't rely upon that API to double-check), and the lowest impact change would be to modify SimpleChannelPromiseAggregator to preserve the first failure (e.g. lastFailure shouldn't be set if it is non-null). An alternative would be to remove constraint (2) and adjust its usage accordingly (e.g. more try/catch and conditional logic to bail early if synchronous failure).

[Scottmitch]

@slandelle - please check #11168 for the proposed fix.

[slandelle]

@Scottmitch Thanks a lot, I'll have a look this afternoon.

However, is changing SimpleChannelPromiseAggregator enough?
I mean:

• if writeHeaders fails: https://github.com/netty/netty/blob/netty-4.1.63.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java#L108
• what's the point of still trying to writeData: https://github.com/netty/netty/blob/netty-4.1.63.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java#L128
• and the trailing headers: https://github.com/netty/netty/blob/netty-4.1.63.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java#L133

It also feels suspicious that the ChannelFutures returned by encoder.writeHeaders and encoder.writeData are just discarded.

[Scottmitch]

However, is changing SimpleChannelPromiseAggregator enough?

This change is sufficient to preserve the first/"original" exception.

what's the point of still trying to writeData... and the trailing headers

You are correct if one operation fails the, subsequent operations are also expected to fail. As described above in (2) a goal was to simplify the control flow and improve readability (assuming exceptions are "exceptional"). This code could be re-written to break out early with more branches/conditionals but you would have to be careful to ensure resources are released (e.g. ReferenceCounted objects) and state machines are properly updated. In practice there should be some form of backpressure applied to avoid/minimize the "stream exceeded" condition (so I assume this condition isn't persistent).

It also feels suspicious that the ChannelFutures returned by encoder.writeHeaders and encoder.writeData are just discarded.

A common pattern in Netty is as follows:

ChannelFuture doWork(...) {
  // allocates a ChannelPromise internally, returns the ChannelFuture to listen for results
  return doWork(..., newPromise());
}

ChannelFuture doWork(..., ChannelPromise p) {
  // do the work, complete p when done
  return p;
}

With this pattern if you provide your own ChannelPromise the returned ChannelFuture isn't necessary to get the results, because you can listen to the ChannelPromise (which extends ChannelFuture).

[normanmaurer]

@slandelle please reopen if you think there is more work todo