Closing channels on request streaming failures

[sanjams2]

Im using the reactive streams mode of the AsyncHttpClient library and have uncovered some behavior I feel may be incorrect.

Take the following example:

1. User creates a request and specifies the body as a Publisher<ByteBuf> (code)
2. Request is executed, body begins streaming
3. During the streaming of the body, an exception occurs. The body publisher's subscriber's onError method (code) is called to signal this error

When this occurs, I am observing that the connection to the external service is left open and no error is signaled (error can probably only be signaled by closing the connection). This means that the external service sits there waiting indefinitely (if it configures some timeout, then until that timeout). The reason this happens is because the NettyReactiveStreamsBody overrides the error behavior and simply aborts the future (thus leaving the channel intact) (override code) (abort code)

Why do I think this is incorrect behavior? Well I am probably wrong, but to me it seems that once there is an error, there is no good way to recover. Assuming the streaming behavior is not replayable (it usually isnt because you are loading the bytes from elsewhere and typically overwriting buffers to save space, hence "streaming"), then this request cannot be saved. If the request cannot be saved, then the "right" thing to do is to signal that to the downstream service being called. This can be done by closing the channel (and subsequently the connection). This may not be ideal in HTTP/2 if multiple requests are using the same channel, but in HTTP/1.1, closing the channel seems appropriate. (last I checked this library didnt support H2?)

I went back and looked at the commit that added the code and its pull request. It actually seems like the decision to call abort instead of cancel was not necessarily an intentional one. Here is the pull request. There is a comment stating "Shouldn't this method be capturing the NettyResponseFuture, and at very least invoking future.abort in the error callback?" which leads me to believe the decision to call abort was not well thought out.

What am I asking? If I am correct, and cancelling the connection is the correct thing to do, then I think this line should be changed to

future.cancel(<force>)

Perhaps if this is an issue when using HTTP/2, a check could be added to only do this if the used protocol is HTTP/1.1

I can attempt to make the change if it is deemed the correct way to handle this case.

Thanks

[sanjams2]

Given #1096 is still open, Im assuming HTTP2 is still not supported