Doesn't work with Express.js

[jinjor]

I wrote simple Express app and got an error.

var fs = require('fs');
var express = require('express');
var app = express();

app.get('/', function (req, res) {
  res.send('hello!')
})

var options = {
  key: fs.readFileSync('./ssl/key.pem'),
  cert: fs.readFileSync('./ssl/cert.pem')
};

require('http2').createServer(options, app).listen(8080);
// require('https').createServer(options, app).listen(8080);// https module works well

_stream_readable.js:505
    dest.end();
         ^
TypeError: undefined is not a function
    at Stream.onend (_stream_readable.js:505:10)
    at Stream.g (events.js:199:16)
    at Stream.emit (events.js:129:20)
    at _stream_readable.js:908:16
    at process._tickCallback (node.js:355:11)

Is that a node-http2's matter or Node's one?
It is hard to debug for me...

Node: v0.12.0
node-http2: v3.2.0
express: v4.11.2

[senditu]

I'm getting the same error.

[molnarg]

Thanks for reporting. This would indeed be nice. I expect to have some time to look into this in about a week. Feel free to send pull requests if you happen to find the root if the issue.

[jinjor]

Now I found it not easy to fix...

Express replaces the request's __proto__ with another object which is similar to IncomingMessage but is not a Stream.
https://github.com/strongloop/express/blob/master/lib/middleware/init.js#L18-L19

So, your IncomingMessage inherits PassThrough at first but soon loses end method.
Maybe other methods are broken too.

[molnarg]

Thanks for digging deeper! I don't have an idea for a fix yet, but will be thinking about possible solutions...

[dougwilson]

Please feel free to open a new bug report in Express.js as well, if there is something we can fix or change to make it work better with http2!

[molnarg]

In node-http2, all of the features can be implemented without relying on inheritance, even without performance loss. Both the IncomingMessage class and inheriting from PassThrough is just convenience, they are not necessary. In contrast, in express, implementing the object augmentation without mangling the proto would result in performance loss. But it needs some work, and the code will be somewhat uglier... I'm still thinking about a prettier solution.

[molnarg]

So, although IncomingMessage and PassThrough are avoidable, both IncomingRequest and IncomingResponse are necessary (or avoidable only with performance penalty). And the protos belonging to these classes would be replaced by express on instances...

Maybe express could check if the proto that it wants to replace is really the built-in IncomingMessage, and if not, it would just copy the augmentation methods to the instance itself instead of the proto replacement. It would be a performance penalty, but only when used with node-http2 (or other alternative HTTP API implementation). @dougwilson what do you think?

[dougwilson]

So if the only reason why http2 doesn't work with Express is because of this prototype stuff, then it should be fixed in 5.0 this summer.

If we are brainstorming on a method to get it into 4.x and also keep backwards compatibility, then we can and a PR would be welcome :) Some things to think of is that with sub apps, we don't know the prototype we want to replace, so you would need to walk down the entire chain searching for IncomingRequest. We also need to think about how to handle things like non-configurable properties, where today there is no issue and Express will overwrite them, but changing to a non-proto-replacement may cause different behavior or errors for people.

In Express 5 we are moving to prototype injection over replacement, though it's not backwards compatible.

[dougwilson]

Also we need to consider that if we put things on the instance itself, it'll change behavior of people doing Object.keys() or hasOwnProperty and the like, so we have to take that into consideration as well, if this change when using http2 will cause issues in various existing middleware modules.

[molnarg]

I would be happy with 5.0 as a solution without support for 4.x.

But, what do you mean by prototype injection? If it is var originalPrototype = req.__proto__; req.__proto__ = app.request; app.request.__proto__ = originalPrototype; then I don't think it would solve the problem.

[dougwilson]

It would not be that code, since that code would not allow for the co-existence of http2 and node.js core requests on the same express app, which is something we want.

[molnarg]

Yeah, it's pretty tricky to implement this thing properly. If express is going to do something like this, then I think both http2 and http1 would work: maintain a map of observed protos (in this case, the built-in IncomingMessage, and node-http2 IncomingRequest), and make a separate injectable proto object for each of them, and always inject the appropriate one. It's up to you of course, just thinking about the proper way of implementing this...

[dougwilson]

One problem with the mapped idea is that we would have to take a dependency on http2 and then, for equality to work, people would have to only use the same version of http2 on which express is getting the http2 prototype reference from.