New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't work with Express.js #100

Open
jinjor opened this Issue Feb 25, 2015 · 43 comments

Comments

Projects
None yet
@jinjor
Copy link

jinjor commented Feb 25, 2015

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

This comment has been minimized.

Copy link

senditu commented Mar 14, 2015

I'm getting the same error.

@molnarg

This comment has been minimized.

Copy link
Owner

molnarg commented Mar 18, 2015

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

This comment has been minimized.

Copy link

jinjor commented Mar 29, 2015

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

This comment has been minimized.

Copy link
Owner

molnarg commented Mar 29, 2015

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

@dougwilson

This comment has been minimized.

Copy link

dougwilson commented Apr 8, 2015

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

This comment has been minimized.

Copy link
Owner

molnarg commented May 3, 2015

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

This comment has been minimized.

Copy link
Owner

molnarg commented May 3, 2015

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

This comment has been minimized.

Copy link

dougwilson commented May 3, 2015

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

This comment has been minimized.

Copy link

dougwilson commented May 3, 2015

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

This comment has been minimized.

Copy link
Owner

molnarg commented May 3, 2015

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

This comment has been minimized.

Copy link

dougwilson commented May 3, 2015

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

This comment has been minimized.

Copy link
Owner

molnarg commented May 3, 2015

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

This comment has been minimized.

Copy link

dougwilson commented May 3, 2015

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.

@molnarg

This comment has been minimized.

Copy link
Owner

molnarg commented May 3, 2015

Yes if the map is static. But it could start as empty (or preloaded with
the builtin http1 IncomingMessage), and dynamically filled up with
proto->injectable_proto mappings as new protos are observed, since it's
easy to create such injectable_protos on the fly.

But anyway, this is probably not the right place to talk about it. Should I
open a ticket in express to keep track of this?
2015.05.03. 21:15 ezt írta ("Douglas Christopher Wilson" <
notifications@github.com>):

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.


Reply to this email directly or view it on GitHub
#100 (comment).

@dougwilson

This comment has been minimized.

Copy link

dougwilson commented May 3, 2015

There is already an open issue in Express that is linked to this issue.

@Restuta

This comment has been minimized.

Copy link

Restuta commented May 15, 2015

@dougwilson would be helpful if you have posted the link here

@crepererum

This comment has been minimized.

Copy link

crepererum commented May 25, 2015

Related Express issue:
expressjs/express#2364

@olalonde

This comment has been minimized.

Copy link

olalonde commented May 31, 2015

@crepererum for some reason that issue was locked and discussion is restricted to collaborators 👎

@stanier

This comment has been minimized.

Copy link

stanier commented Jun 3, 2015

@olalonde Well let's hope that means something is being done about it. This pretty much breaks HTTP/2's implementation into ExpressJS, further holding back nodeJS applications to HTTP/1.1

I'm actually surprised that it's taking this long to get a reliable implementation of HTTP/2 going.

@MrOutput

This comment has been minimized.

Copy link
Contributor

MrOutput commented Jun 4, 2015

@stanier me too. Surprised core node doesn't support HTTP2 already.

@stanier

This comment has been minimized.

Copy link

stanier commented Jun 4, 2015

@ralph3991 imho, Joyent's node has been slower on releases. I'm more surprised that io.js hasn't made a decision on it. They seem rather in limbo on how to deal with it.

@MrOutput

This comment has been minimized.

Copy link
Contributor

MrOutput commented Jun 4, 2015

@stanier wow, I didn't even know that existed. Thank you. I checked it out and it looks much better than node for the cutting edge. Yeah id have to agree again, they support ECMAScript 6 but not HTTP2? Hmm. I'm just happy rfc7540 is finally finished and developers can start making final releases. I checked out https://github.com/molnarg/node-http2 but they are still on Draft 16 :(

@nwgh

This comment has been minimized.

Copy link
Collaborator

nwgh commented Jun 4, 2015

@ralph3991 not true, as of v3.2.0, node-http2 advertises h2, not draft 16 (any documents that say otherwise are just out of date - pull requests welcome to fix it!)

Also, this conversation is quite the derail from the actual purpose of this issue. If it continues, I may have to lock the issue.

@MadLittleMods

This comment has been minimized.

Copy link

MadLittleMods commented Jun 10, 2015

👍

@mattfysh

This comment has been minimized.

Copy link

mattfysh commented Jun 12, 2015

any updates on this? last comment on the express thread is to raise the issue here, does not sound like they are going to fix. in the meantime we're using connect directly

@dougwilson

This comment has been minimized.

Copy link

dougwilson commented Jun 12, 2015

This is because people are linking to the wrong thread in the Express repo; the correct thread is listed right at the top of this issue:

thread

There is no backwards-compatible way to fix this in Express, so without anything happening here, it won't be until Express 5.0 that this module (and any other server that does not inherit from Node.js core http prototype) will work with Express.

@CodeTheInternet

This comment has been minimized.

Copy link

CodeTheInternet commented Jun 20, 2015

@molnarg, is a solution being worked on in this repo, or are we to wait for Express v5 and hope it resolves any issues?

@stanier

This comment has been minimized.

Copy link

stanier commented Jun 21, 2015

@CodeTheInternet, assume that responsibility for the issue is being thrown on @strongloop

@syzer

This comment has been minimized.

Copy link

syzer commented Jun 23, 2015

+1

@amitmtrn amitmtrn referenced this issue Jul 17, 2015

Open

Release 5.0 #2237

25 of 42 tasks complete
@mavrick

This comment has been minimized.

Copy link

mavrick commented Sep 8, 2015

Please ignore my last comment - apologies.

@jabrena

This comment has been minimized.

Copy link

jabrena commented Sep 23, 2015

I would like to create an example with this dependency.
What Router library similar to express does exist and it doesn't have any problem?

@arashthk

This comment has been minimized.

Copy link

arashthk commented Sep 23, 2015

@jabrena connect seems to work fine with it

@gajus

This comment has been minimized.

Copy link

gajus commented May 6, 2016

Using:

"express": "^5.0.0-alpha.2",
"http2": "^3.3.4",
const http2 = require('http2');
const express = require('express');
const yargs = require('yargs');
const path = require('path');
const fs = require('fs');

const argv = yargs
      .help()
    .strict()
    .options({
        port: {
            default: 8000,
            demand: true,
            type: 'number'
        }
    })
    .argv;

process.on('unhandledRejection', (reason, promise) => {
    /* eslint-disable no-console */
    console.log('Unhandled RejectionPromise');
    console.log('promise', promise);
    console.log('reason', reason);
    console.log('reason.stack', reason.stack);
    /* eslint-enable */
});

const app = express();


app.get('/', (req, res) => {
    res.json({foo: 'test'});
});

/* eslint-disable no-console, no-unused-vars */
app.use((err, req, res, next) => {
    console.error(err.stack);
    /* eslint-enable */

    res
        .status(500)
        .send('Internal Server Error');
});

http2
    .createServer({
        log: require('bunyan').createLogger({name: 'myapp'}),
        key: fs.readFileSync(path.resolve(__dirname, './localhost.key')),
        cert: fs.readFileSync(path.resolve(__dirname, './localhost.crt'))
    }, app)
    .listen(argv.port, (err) => {
        if (err) {
            throw new Error(err);
        }

        /* eslint-disable no-console */
        console.log('Listening on port: ' + argv.port + '.');
        /* eslint-enable no-console */
    });

Does not work.

The complete log output: https://gist.github.com/gajus/843fd2d682f946680af161b7054b01f2

@MrOutput

This comment has been minimized.

Copy link
Contributor

MrOutput commented May 6, 2016

whats up with babel.js in there?

@gajus

This comment has been minimized.

Copy link

gajus commented May 7, 2016

@MrOutput No relevance. Add syntax support for object destructuring.

@JakeChampion

This comment has been minimized.

Copy link

JakeChampion commented May 11, 2016

@gajus could you put that big log code block into a gist at all? It is making the page very large.

@azat-co

This comment has been minimized.

Copy link

azat-co commented Jul 7, 2016

@gajus please hide you code

For now (while we are waiting for Express v5) you can use spdy with express

const port = 3000
const spdy = require('spdy')
const express = require('express')
const path = require('path')
const fs = require('fs')

const app = express()

app.get('*', (req, res) => {
    res.json({foo: 'test'})
})
let options = {
    key: fs.readFileSync(__dirname + '/server.key'),
    cert:  fs.readFileSync(__dirname + '/server.crt')
};
console.log(options);
spdy
  .createServer(options, app)
  .listen(port, (err) => {
      if (err) {
          process.exit(1)
      }
      console.log('Listening on port: ' + port + '.')
  })
@konradjurk

This comment has been minimized.

Copy link

konradjurk commented Jul 15, 2016

@azat-co SPDY is dead

@azat-co

This comment has been minimized.

Copy link

azat-co commented Aug 9, 2016

@konradjurk what exactly do you mean by dead? spdy repo is alive. even if there's not much active development, you can use it right now

see the official h2 spec where under Node it says http2 and spdy: https://github.com/http2/http2-spec/wiki/Implementations

@konradjurk

This comment has been minimized.

Copy link

konradjurk commented Aug 9, 2016

@azat-co Google Chrome doesn't support SPDY anymore: http://blog.chromium.org/2016/02/transitioning-from-spdy-to-http2.html

So when even its inventor won't support it you know it is dead

@azat-co

This comment has been minimized.

Copy link

azat-co commented Aug 9, 2016

@konradjurk Google SPDY become HTTP/2, if my understanding correct. That's the standards. Now the libraries: spdy library works just fine as it is. It's not dead. The repo is not down. The repo does NOT say it is deprecated. http2 lib for node is not supporting Express so spdy is a good option. The official H2 doc&specs list spdy as one of the implementations.

@azat-co

This comment has been minimized.

Copy link

azat-co commented Aug 9, 2016

@konradjurk so basically spdy become http/2 standard and whatever spdy lib had is a viable choice for H2 features... spdy lib is not dead in any case AFAIK 😄

@nwgh

This comment has been minimized.

Copy link
Collaborator

nwgh commented Aug 9, 2016

OK, this is getting out of hand... too much discussion not related to fixing this bug spamming my inbox. I'll be locking this conversation.

However, just to note, spdy is dead. Chrome no longer supports it. I disabled support for it in Firefox 50 (scheduled to release in 12 weeks or so), and removed it entirely in Firefox 51 (6 weeks or so after 50). It is not compatible with h2, even if it was the basis for the spec work, so no, spdy is not a good option for anyone.

If I ever get more than a few hours to work on this repo, I might actually try to make this work with Express since it's obviously a pain point for a lot of people, but I don't see any time soon when that would happen. Until that, patches are more than welcome to fix the issue.

Repository owner locked and limited conversation to collaborators Aug 9, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.