ISPN-14571 Propagate errors in REST chunked response #10693
Conversation
|
While the changes themselves seem fine, I am not sure if this what we want to do. From I have seen many clients don't pay attention to trailer headers. The safest may be just to kill the connection, but maybe someone with more HTTP experience could chime in. |
|
I would vote to kill the connection instead of using trailer headers, but as I mentioned previously I am open to either suggestion. I just can't imagine a client would know to check this specific trailer header for an exception. Where as if the client dies, they will try again and realize it disconnected again, so maybe something is wrong. And also since this is logged as an ERROR it should be more than visible in the server log file. |
|
While changing the behavior to close the connection, everything works fine with HTTP 1.1. With HTTP 2.0, we have a slightly different behavior. The server sends back to the client a frame for the stream canceled and then closes. OkHttp has a bug with that, entering into an infinite retrying loop. We can work around that by not retrying requests when using HTTP 2.0. I need to check if we can disconnect the client with HTTP 2.0 without sending the cancel stream. But I am afraid that could cause an issue with other HTTP clients. |
I don't think so. Not sending a cancel stream would be like the socket was killed by something else, which is perfectly fine for behavior here as we aren't doing this for correct propagation but rather because it is an error we can't recover from and this is the only way we can notify the client that something really went wrong. |
| @@ -116,6 +116,8 @@ public RestClientOkHttp(RestClientConfiguration configuration) { | |||
| } else { | |||
| builder.protocols(Arrays.asList(Protocol.HTTP_2, Protocol.HTTP_1_1)); | |||
| } | |||
| // OkHttp might retry infinitely on HTTP/2. | |||
| builder.retryOnConnectionFailure(false); | |||
There was a problem hiding this comment.
When we have HTTP/2 and close the connection on the server, OkHttp will wrap the exception into a StreamResetException and retry infinitely.
There was a problem hiding this comment.
Is this needed if the CANCEL frame isn't sent?
There was a problem hiding this comment.
For both cases :/ If the server sends the CANCEL frame, OkHttp creates the StreamResetException. If the server closes the connection, OkHttp swallows the EofException and turns it into a StreamResetException. So, in either case, it ends up retrying infinitely.
| return; | ||
| } | ||
|
|
||
| unsafe.closeForcibly(); |
There was a problem hiding this comment.
Calling ctx.close() or the closeForcibly() directly from the channel calls the AbstractHttp2StreamChannel.close(...) to send the CANCEL frame before closing the connection.
There was a problem hiding this comment.
So there is no difference between ctx.close and unsafe.closeForcibly?
There was a problem hiding this comment.
When using HTTP 2, yes. The methods are https://github.com/netty/netty/blob/24a0ac36ea91d1aee647d738f879ac873892d829/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2StreamChannel.java#L715-L717 and calling ctx.close() will invoke https://github.com/netty/netty/blob/24a0ac36ea91d1aee647d738f879ac873892d829/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2StreamChannel.java#L655-L657.
With this approach here, of calling the parent, it seems to be closing without sending the cancel frame.
There was a problem hiding this comment.
Maybe I am misreading, but it seems there is a difference that calling unsafe.closeForcibly() doesn't produce the cancel frame but calling ctx.close() would produce the frame right?
There was a problem hiding this comment.
Yeah, but on the parent unsafe in the case of HTTP 2. From debugging:
ctx.close()andctx.channel().unsafe().closeForcibly()=> Both callsAbstractHttp2StreamChanneland produces the frame;ctx.channel().parent().close()andctx.channel().parent().unsafe().closeForcibly()=> Do not produce the cancel frame.
There was a problem hiding this comment.
Oh okay, the key difference is the parent then.
There was a problem hiding this comment.
Should we check the parent in the first if then instead? Or does the HTTP2 first if check not pass?
There was a problem hiding this comment.
Changing the check order causes issues with HTTP/1.
There was a problem hiding this comment.
I removed all the checks, and now it is just calling ctx.close() for simplicity. When using HTTP/1, all is good. If it is HTTP/2, it sends the cancel the frame first, but it ends up closing the connection after.
|
Integrated into main, thanks @jabolina ! |
https://issues.redhat.com/browse/ISPN-14571
It doesn't seem the nicest way. If an error happens midway through writing the chunked response (key and entries), we cancel the stream, write HTTP last content, and add a trailer header with the exception message.