Setting a deprecated header should not break

[felicienfrancois]

The current behavior prevent from simply replacing https by http2 in existing peace of software

It should not throw a breaking exception. Instead It should:

• log a deprecation warning
• either set the header anyway (if setting it has no impact), or drop it (if it is no longer meaningful) or migrate it if it is replaced by new header(s).

But in all cases, the request should be processed

[nwgh]

I'm not so sure - there's no better way to get someone to fix their code than by throwing a breaking exception! Regardless, if we do go the route of not throwing an exception, we should in no case set a deprecated header - there are other endpoints that will get mad at us if we send one.

Also, what header (specifically) are you setting that causes the exception? There is, perhaps, a better alternative that can/should be used in all cases.

[felicienfrancois]

I get this error on the following headers:

• host on requests
• transfer-encoding and connection on responses

Here more arguments in favor of not breaking:

• There are use cases where headers are set by a third party piece of software (middlewares/plugins/modules). So in order to be able to use http2 in such a project, we have to wait for all third party middlewares/plugins/modules to be compliant which will maybe makes people fix their code a day but in the meantime it will surely prevent adoption of http2.
• Breaking on deprecation is not a common behavior and so is not expected here. Deprecated !== Invalid. Deprecated means it SHOULD no longer be used. SHOULD !== MUST NOT
• All http2 classes extends EventEmitter. When an error occurs the expected behavior is to emit an error event so it can be handled, not to throw an exception
• I and most people expect robustness from an http server. It should never break. It may log errors and warning but never break. If I cannot trust the http server I will never use it in production. If it breaks on a simple harmless header set on what else could it break ?
• An http server handles transmission of data using a protocol. Its role is not to validate the content. Headers are part of the content of the request. If I want to set any header, even if it is deprecated according to the RFC, I should be able to do it.

Also, as far as I understood, http2 is not only an HTTP/2 server but an HTTP server that supports HTTP/2. http2 also handles HTTP 1.1 request/response, transparently, using the same event handlers right ? If so, HTTP 1.1 headers deprecated in HTTP/2 should not be breaking in http2 which have to handle both protocols transparently.

Also, When I write this

http2.request({
  hostname: 'myhost.com',
  port: 443,
  path: '/',
  method: 'GET',
  headers: {
    'Some-Deprecated-Header': 'SomeValue'
  }
}, function(response) {

});

How can you know if the request will be HTTP/2 or HTTP 1.1 before connecting to the server?

[muzuiget]

I am using http-proxy in development environment(with gulp-connect) as reverse proxy for the api-server. This behavior will break the connection, because http-proxy auto add connection to the request, and it can't override in client-side code.

https://github.com/nodejitsu/node-http-proxy/blob/1.13.2/lib/http-proxy/passes/web-outgoing.js#L41-L47

I agree there at least an option to let user code to configure this behavior, handle both protocols transparently.

[olsonpm]

I got here trying to use weinre. I didn't debug the problem fully - but I haven't had this issue until just now trying to hook weinre up.

It's an old project, I wouldn't expect it to update deprecated headers. It's also an extremely useful tool however and this behavior is preventing me from using it.

I do appreciate the http2 project though. It's been awesome to me :)

[surma]

I am investigating this right now as I have encountered a site that uses Host headers with their push promises. I think throwing is not the right behavior.

I found the following comment in node-http2’s the code:

  // * An HTTP/2.0 request or response MUST NOT include any of the following header fields:
  //   Connection, Host, Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade. A server
  //   MUST treat the presence of any of these header fields as a stream error of type
  //   PROTOCOL_ERROR.

Looking at the HTTP/2 spec, the wording is different:

Clients that generate HTTP/2 requests directly SHOULD use the :authority
pseudo-header field instead of the Host header field.

A little further up in the spec:

This means that an intermediary transforming an HTTP/1.x message to HTTP/2 
will need to remove any header fields nominated by the Connection header field, 
along with the Connection header field itself. Such intermediaries **SHOULD** also 
remove other connection-specific header fields, such as Keep-Alive, Proxy-Connection, 
Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection header field.

This obviously only loosely applies to node-http2, but I think it shows that the spec intends for clients to not break if these headers get sent. Therefore, I think it would be good for node-http2 to not break on deprecated headers but issue a warn-level log instead.

I’ll gladly open a PR for this if the maintainer agree that this is an acceptable approach.

[olsonpm]

Thanks a ton for mentioning the comment vs spec language. Interesting because specs don't change once published, but the comment reads as if it were copy/pasted.

[pavel]

@nwgh

I'm not so sure - there's no better way to get someone to fix their code than by throwing a breaking exception!

I disagree, because someone might be a 3rd party HTTP/1 server, for example, CouchDB, which uses only HTTP/1. You're forgetting that node-http2 can and will be used as a proxy and for now mostly as HTTP/2 to HTTP/1. This should be focused around actual usage and not "programming best practices". You can add strict mode for those who want to be as close to spec as possible.

[DaSchTour]

@pavel If I understand it right I currently have exactly this scenario. I'm using BrowserSync to proxy our application server and serve static frontend resources. There would be a lot to do on the application server to migrate to HTTP/1 and I only would like to use http2 for serving the mass of small JS modules through SystemJS a bit faster, so I could speed up frontend development. But this error doesn't allow me to use http2 module in browsersync.

[pavel]

@DaSchTour You can use node-spdy instead as it does handle such cases. (I assume that the reader understands that this advice is not advertisement of node-spdy, and just a library to workaround the issue. No comparison should be implied from my comment).

BrowserSync usage example:

// First run: npm install browser-sync spdy
const bs = require('browser-sync').create();

bs.init({
    server: './app',
    httpModule: 'spdy',
    https: true
});

[DaSchTour]

@pavel thanks for the workaround. Since SPDY is some kind of subset of HTTP2 this is a fair suggestion. In the longterm we all will migrate to HTTP2 as this is the new standard. But It's a long road to go. So luckily there are some "bridge" technologies :D Sadly Chrome already removed support for SPDY 😞

[pavel]

@DaSchTour Actually node-spdy provides HTTP/2 support. Though the name mentions spdy HTTP/2 support is provided as well. From node-spdy github page:

With this module you can create HTTP2 / SPDY servers in node.js with natural http module interface and fallback to regular https (for browsers that don't support neither HTTP2, nor SPDY yet).

[Yugloocamai]

@pavel completely agree, this has nothing to do with broken code. Some services just use different protocols, and to say that a service that isn't using HTTP2 is broken is pretty bold.

[nwgh]

This will not be fixed. node-http2 only speaks http/2, and if something is speaking http/2 and sending a deprecated header, that something is broken.