master, added an option to "request" of http.js to enable to choose the protocol version

[yadutaf]

Hi again,

I came to an issue while trying to proxy requests from wget: it understands only HTTP 1.0. While the current version of the code is able to answer him with the right version, it doesn't enable the user to choose protocol version for the proxyed request.

With this pull request, i try to introduce this feature as smoothly as possible in the code.

thanks

[bnoordhuis]

Looks good. Still needs tests and documentation (same goes for #1749 but you or we can probably back-port them whole). It should update lib/http2.js as well, it only patches the legacy http lib now.

[koichik]

@jtlebi - HTTP/1.1 proxy can support HTTP/1.0 client.
In this case, proxy must delete hop-by-hop headers (especially Transfer-Encoding) from server's response.
Please refer to #1234 and http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-14#section-7.1.3

[yadutaf]

I understand this. By design I just wanted to induce the lowest overhead possible. This is why I chose a streaming approach. It almost as transparent as an inspection layer. I just inspect the "host" header to redirect to the right application.
Nonetheless, i may reconsider my position if it raises more problems than it actually solves:)

[mnot]

What are you trying to fix here?

As koichik noted, the HTTP version is not end-to-end; node is already behaving correctly.

[bnoordhuis]

Oh, I may have misunderstood the issue. I thought the purpose of this patch was to allow talking to an overzealous HTTP/1.0 proxy (something I've run into in the past) and that it accomplishes. Is this something we want?

[yadutaf]

I'm sorry, i may have mis-explained my suggestion.

In a piece of sofware i work on, i missed an option to force a request to be issued with a specific HTTP version. In fact, this is more a "Pull suggestion" than a "Pull request" :)

Now, to be more precise about what I am trying to achieve in my personal project: This is a very very lightweight node.js reverse-proxy wich is almost fully transparent to both ends. By this small addition (which doesn't "fix" anything), i avoided to implement an http/1.0 <---> http/1.0 conversion layer only for the buggy Wget.

That's all. Sorry for the misunderstanding.

[bnoordhuis]

No worries, I should learn to read. I'll leave this pull request open for a bit. Your patch, intended or not, addresses the issue of too stringent HTTP/1.0 proxies. They should be exceedingly rare by now but if enough people pipe up, we might still merge it.

[ry]

@mikeal - opinions?

[koichik]

@jtlebi - An HTTP/1.1 server may send back HTTP/1.1 response to an HTTP/1.0 client.
Node's http.Server (i.e. your proxy) works so.
If your proxy uses HTTP/1.0 client, it receives either HTTP/1.0 or HTTP/1.1 response from the server.
But it always sends HTTP/1.1 response to the client.
Is this transparent?

@bnoordhuis, @ry - I am +1 to support HTTP/1.0 client, but -1 to add httpVersion argument to http.Client.request() because it is a legacy API.

[yadutaf]

@koichik - I think it may be considered as transparent as soon as the answering server is not using HTTP/1.1 only features like "transfer-encoding: chunked". This is actually the behavior followed by Apache2 when answering to Wget.

@ALL - Do you have any draft doc of the new HTTP API so that I can easily provide an updated patch ?

[mikeal]

I'm having a hard time remembering all the eccentricities of HTTP 1.0 but I remember there being more than just shifting the encoding and disabling keepalive.

First off, this needs to be patched against http2.js not http.js, sorry for the confusion but http2.js is used by default in master now. @ry can we remove http.js at this point? All the tests were passing for a while before we broke all the tests with the libuv merge.

The documentation in master relates to the latest http, which is http2.js as of 0.5.3 and using the http.request() as of 0.4.0.

@koichik +1 http.request() should be the only way to pass this option. @jtlebi see exports.request and the ClientRequest constructor for your entry points. Don't worry about the Client object, it's legacy and is now only a shim.

ClientRequest and ClientResponse need to have the version attached to it so that if you pipe() an instance to/from another Stream it can capability detect for HTTP 1.0.

Once you handle all the issues we've brought up I think that we should merge this knowing that it most likely won't be complete. The only way to find all the eccentric 1.0 bugs will be to use it in the wild for a while. When they do surface, and they will, we should be diligent about adding tests. Also, this patch needs tests :)

PS. @jtlebi while it's definitely more difficult, in your own reverse proxy project you may want to look at using a TCP server/client and hooking in to the HTTP parser the way http2.js does. It would definitely be faster and likely allow you to handle issues like this more freely. It will definitely be harder to write though :)

[ry]

I am hesitant to add the ability to pretend we're an old version. So far I see no justification.

Can you please show us how to reproduce the problem you're seeing?

[mikeal]

@ry are we really pretending? this is the client, so as long as we disable 1.1 encoding features aren't we basically a 1.0 client?

if it was the server i wouldn't like pretending to be 1.0 when the parser is most definitely not 1.0.

if the server responds with a 1.1 request we'll parse it as 1.1, which means it's basically been forcibly upgraded. is this the right thing to do? now that i think about it, it might not be. it's also late in Berlin and I've been drinking ;)

[yadutaf]

I added a preliminary port to the new API. Still needs testing.
It includes both the legacy and the new API. Tell me if I missed the entryPoint :)

[mikeal]

these lines are over the max column width, which is 80. we're more stringent about new contributions adhering to the column width than older code.

[koichik]

I do not know HTTP/1.0 in detail, but probably:

• ClientRequest should not use default (global) Agent (only http2.js).
• ClientRequest should not set Host header by default (both http.js and http2.js).

It may not be only these.

[mikeal]

HTTP/1.1 accepts a host header, and we're likely to know which one to set, and already have an option to disable host header setting, so I thinks it fine to leave that feature on.

+1 instead of ifdefing 1.0 everywhere for keepAlive we can just default to agent:false in the constructor.

[ry]

We have not yet seen justification for setting http version in requests. This is not going in.

[bnoordhuis]

Okay, closing the issue.

@jtlebi: Thanks anyway.