RxNetty HTTP client protocol always sends a empty message-body for GET method.

[kurzweil]

I am using RxNetty to implement a HTTP client which performs HTTP GET and WebSocket operations. Currently I'm using this client to interact with the stock remote debugging interface implemented as a webserver internal to the Chrome Browser and have discovered that RxNetty expresses a bug within the Chrome Browser that immediately causes it to segmentation fault while upgrading to the WebSocket protocol

I believe this might be because the RxNetty client isn't explicitly following the RFC7230 where HTTP-messages may have a message-body but are not required in all cases and in cases of the GET method should not have a message-body. Obviously, the Chrome Browser shouldn't crash over this subtle protocol infringement, but I don't think RxNetty should be explicitly sending empty bodies either. (Roy Fielding has an opinion about GET methods with message-bodies)

I've been able to for RxNetty not to add the explicit message-body by temporarily commenting out rxnetty-http/src/main/java/io/reactivex/netty/protocol/http/client/internal/RawRequest.java#L180
which enables Chrome to continue with the upgrade and not crash. I've captured a before and after look of the protocol exchange when commenting out the line.

Any thoughts?

Much appreciated,
Ken

[jamesgorman2]

Hey Ken, looks like a duplicate of #470. Short version is that the RFC seems to conflate message body and payload (and null-ability and emptiness - is an zero length body the same as a single zero length chunk?) w.r.t. chunked encoding so it's ambiguous as whether RxNetty is 'correct' (I tend to side with Nitesh on the loose interpretation. Of course this doesn't help if Chrome is dying on you).

on the tech side, from Nitesh:

In order to make sure that we do not put Transfer-Encoding: chunked for empty bodies, we would have to delay writing headers until the content stream emits an item or terminates. Although possible, I think it does not achieve much benefit. A server that handles chunked encoding, must be able to handle such requests correctly if the request is correctly having the last chunk.

I would be interested in seeing if there are any servers that restricts such requests. I haven't seen any.

[NiteshKant]

Thanks @kurzweil for the detailed analysis ❤️

@jamesgorman2 thanks for pitching OSS FTW ❤️

Since I have now seen an example of things that behave badly with such requests, its time to atleast put a workaround 😄

I think if a Content-length header is set to 0, then RxNetty should not send chunked-encoded payload and hence avoid an empty chunk. I can make this change, if it isn't the case today. WDYT?

[kurzweil]

Setting the Content-length header to 0 suppresses generation of a message-body. For my purposes this a suitable workaround. Thanks for the help.

[jamesgorman2]

EDIT: I've been working in .Net land recently and just realised Observable is a class not an interface - this mean this won't work as is since the header objects would need to extend Observable<HttpClientResponse<O>> and Observable<Void>. Maybe a #withEmptyBody() to set the Content-length and convert the type to Observable is a sufficient start.

Given there is a mechanism to suppress the message body with Content-length: 0, can we put in HttpClientRequestHeader and HttpServerResponseHeader interfaces?

If you had something like

class HttpClientRequest<I, O> extends Observable<HttpClientResponse<O>> implements HttpClientRequestHeader<I, O>

and

class HttpServerResponse<C> extends ResponseContentWriter<C> implements HttpServerResponseHeader<C>

with

HttpClientRequestHeader<I, O> HttpClientRequest<I, O>#withEmptyBody()

and

HttpClientRequest<I, O> HttpClientRequestHeader<I, O>#withBody()

to flip between writable and un-writable requests and set/unset the Content-length header (etc for responses).

You could then have:

class InterceptingHttpClientImpl<I, O> extends InterceptingHttpClient<I, O> {

    public HttpClientRequestHeader<I, O> createGet(String uri) {
        return createRequest(HttpMethod.GET, uri).withEmptyBody();
    }

}

and someone with an impolite API to call could use

  HttpClient<I, O> getThatExpectsMessageBody = HttpClient.newClient(...).createGet(...).withBody();

Not sure about an equivalent for the server responses (ie to make getting empty responses easier for 201, etc)

This is all off the top of my head, so it needs some polishing and might make more sense in code. @NiteshKant Does this fit in with where you're taking RxNetty? I can work up a PR in the week for it if you like

[NiteshKant]

@jamesgorman2 I was thinking more like an implementation detail that does not write a body if Content-Length is 0.
The reason why I wasn't thinking about extending the API is because it really is a workaround, most of the cases, no one should really be worrying about it. So, it doesn't seem to me that it warrants an API change.

For this workaround, the usage will just be:

        HttpClient.newClient(serverAddress)
                  .createGet("/hello")
                  .setHeader(HttpHeaderNames.CONTENT_LENGTH, 0)

instead of

        HttpClient.newClient(serverAddress)
                  .createGet("/hello")

I would think of the change be in RawRequest where the content concatenation is short-circuited if Content-length is set to 0, very similar to what @kurzweil tried. We should do similar change in ContentWriterImpl.
It would be fantastic if you can work up a PR for the change :)

[jamesgorman2]

I can certainly look at this. I'll put something in for the basic solution you proposed.

W.r.t. my proposal, it was around (a) providing sensible defaults for GET, (b) providing a fluent way to create explicit empty messages, and (c) preventing people thinking they can write to the object after making an empty message. These are in-effect separate from actually ensuring the body is empty so I'm happy to leave them (though I might still play around with some of the ideas anyway - I have a related problem with types for some stream-wise JSON handling I'm working on).

[NiteshKant]

though I might still play around with some of the ideas anyway

Absolutely, I will be happy to look at your changes!

[jamesgorman2]

Turns out didn't need a complicated solution and the building blocks were there already.

[NiteshKant]

So looks like adding a header of Content-Length as 0 works today i.e. it won't send the empty chunk. @kurzweil that should unblock you for what you are doing.

@jamesgorman2 I think your PR still makes sense as it is solving a different usecase which is adding Content-Length as 0 when someone hasn't added a content stream (it doesn't work if someone adds an empty stream though, which is fine)

[NiteshKant]

Fixed via #522