Fix Chunked APIs sending incorrect responses to HEAD requests #92042
Conversation
Response bodies must always be empty for HEAD requests. Since the request encoder does not know that its dealing with a response to a HEAD request we have to indicate this fact to it. Also, needed to adjust the test http client to use the http-codec so it is able to correlate what responses are meant for HEAD requests and will correctly read responses for HEAD requests. Without this change the added test reproduces the extra bytes and fails with an assert about more than one response received. closes elastic#92032
original-brownbear
added
>bug
:Distributed/Network
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi @original-brownbear, I've created a changelog YAML for you. |
| .addLast("encoder", new HttpResponseEncoder()) | ||
| .addLast("encoder", new HttpResponseEncoder() { | ||
| @Override | ||
| protected boolean isContentAlwaysEmpty(HttpResponse msg) { |
There was a problem hiding this comment.
Same fix of sorts is used by vert.x, see discussion in netty/netty#6761
| protected boolean isContentAlwaysEmpty(HttpResponse msg) { | ||
| // non-chunked responses (Netty4HttpResponse extends Netty's DefaultFullHttpResponse) with chunked transfer | ||
| // encoding are only sent by us in response to HEAD requests an must always have an empty body | ||
| return msg instanceof Netty4HttpResponse && HttpUtil.isTransferEncodingChunked(msg) |
There was a problem hiding this comment.
It's a little dirty to guess things like this, but it seemed nicer than unnecessarily adding another field to the netty response implementation.
There was a problem hiding this comment.
Seems ok but would it also work to just check if there's anything in the body (i.e. ((Netty4HttpResponse)msg).content().isReadable())? At least IMO we should assert there's no body here.
There was a problem hiding this comment.
++ added the assertion
| protected boolean isContentAlwaysEmpty(HttpResponse msg) { | ||
| // non-chunked responses (Netty4HttpResponse extends Netty's DefaultFullHttpResponse) with chunked transfer | ||
| // encoding are only sent by us in response to HEAD requests an must always have an empty body | ||
| return msg instanceof Netty4HttpResponse && HttpUtil.isTransferEncodingChunked(msg) |
There was a problem hiding this comment.
Seems ok but would it also work to just check if there's anything in the body (i.e. ((Netty4HttpResponse)msg).content().isReadable())? At least IMO we should assert there's no body here.
|
Are we going to backport this to 8.6 (and maybe even 8.5?). Seems safe enough, although it might not be a very impactful bug it'd be embarrassing for someone to hit it in the months before 8.7 comes out. |
original-brownbear
added
auto-backport-and-merge
|
Sure lets do 8.6 I'd say. It's almost impossible to hit this as you point out, maybe not worth adding the noise to 8.5 but 8.6 seems fine. |
…c#92042) Response bodies must always be empty for HEAD requests. Since the request encoder does not know that its dealing with a response to a HEAD request we have to indicate this fact to it. Also, needed to adjust the test http client to use the http-codec so it is able to correlate what responses are meant for HEAD requests and will correctly read responses for HEAD requests. Without this change the added test reproduces the extra bytes and fails with an assert about more than one response received. closes elastic#92032
…92042) (#92049) * Fix Chunked APIs sending incorrect responses to HEAD requests (#92042) Response bodies must always be empty for HEAD requests. Since the request encoder does not know that its dealing with a response to a HEAD request we have to indicate this fact to it. Also, needed to adjust the test http client to use the http-codec so it is able to correlate what responses are meant for HEAD requests and will correctly read responses for HEAD requests. Without this change the added test reproduces the extra bytes and fails with an assert about more than one response received. closes #92032 * fix compile
Response bodies must always be empty for HEAD requests. Since the request encoder does not know that its dealing with a response to a HEAD request we have to indicate this fact to it. Also, needed to adjust the test http client to use the http-codec so it is able to correlate what responses are meant for HEAD requests and will correctly read responses for HEAD requests.
Without this change the added test reproduces the extra bytes and fails with an assert about more than one response received.
closes #92032