http: ignore upgrade header when no upgrade listener is present

[Timothy-]

If a server supports HTTP/2 it sends an "Upgrade: h2,h2c" header.
This should be ignored unless the client actually supports HTTP/2.

This is a breaking change for users that depend on the original behaviour.

[SinisterRectus]

Is there any way you can make this not breaking? Maybe an initialization option for automatic upgrading that is true by default?

[SinisterRectus]

Although maybe that's not a great idea if this is correcting behavior. Is this upgrade behavior documented somewhere?

[Timothy-]

I've changed the implementation to be more in line with nodejs:
https://nodejs.org/dist/latest-v12.x/docs/api/http.html

The normal http.request(opts, cb) should still work and will ignore an upgrade header in the response. You can also listen on the response event: req:once('response', cb)

The upgrade and connect are special cases since the http codec must be detached. In these cases the 'response' event will not be emitted.

If you want to implement websockets, you need to listen for an upgrade request:
req:once('upgrade', cb)
If you want to use http CONNECT, you need to listen for the connect event:
req:once('connect', cb)

[SinisterRectus]

Could you explain what this count check is for?

[Timothy-]

See nodejs http client source.

If you are listening on "upgrade" it means that you expect an upgrade event. If you are not listening on that event, the upgrade header should be ignored. If you are not listening on "upgrade" and the server somehow sends a 101 "switching protocols" then the connection will be destroyed.

Nodejs doc:

Emitted each time a server responds to a request with an upgrade. If this event is not being listened for and the response status code is 101 Switching Protocols, clients receiving an upgrade header will have their connections closed.

[Timothy-]

AppVeyor finally passed.
It seems for windows setting host = '127.0.0.1' in a http.request is required, but not for linux.

[SinisterRectus]

Sorry for the lack of attention. I only use coro-http, so the behavior of this module is not something with which I am familiar. If someone else can confirm that this change is sufficiently implemented and non-breaking, we can pull it and maybe bump and release.

[squeek502]

I think this is a worthwhile change. Just ran into it and the current functionality basically creates a silent failure on Upgrade headers being received (you get a response in the callback but then nothing else happens).

I think it's worth fixing the 'normal' functionality here (assuming most people are using http for http requests) in exchange for potentially breaking backwards compatibility with the abnormal cases (websockets via the http module, CONNECT).