express icon indicating copy to clipboard operation
express copied to clipboard

Node http2 - cannot read this.socket.readable

Open PiniH opened this issue 6 years ago • 26 comments

Testing node's new http2 native module I couldn't get express to serve requests over http2, Using node master build with --expose-http2 flag:

const express = require('express');

const app = express();

app.get('/express', (req, res) => {
  res.send('Hello from express');
});

const server = http2.createSecureServer({
  key,
  cert
}, app);

When requesting /express the server crashed with the following error:

(node:80731) ExperimentalWarning: The http2 module is an experimental API.
_http_incoming.js:104
  if (this.socket.readable)
                 ^

TypeError: Cannot read property 'readable' of undefined
    at IncomingMessage._read (_http_incoming.js:104:18)
    at IncomingMessage.Readable.read (_stream_readable.js:431:10)
    at IncomingMessage.read (_http_incoming.js:96:15)
    at resume_ (_stream_readable.js:811:12)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

PiniH avatar Aug 07 '17 17:08 PiniH

Hi @PiniH since it's experimental and just landed on master, I don't think we have done any research into what would need to be done to support it (either in Express.js or maybe bugs in Node.js experimental code). If you would like to help us and take on that task, that would be amazing :) !

dougwilson avatar Aug 07 '17 17:08 dougwilson

I would love to help investigate and assist with all the investigation/integration between the new http2 and express, however I would need guidance on how/where to start. @jasnell

PiniH avatar Aug 07 '17 17:08 PiniH

Yea, I'd need some time to take a look at what is going on to get some pointers for where to start.

dougwilson avatar Aug 07 '17 17:08 dougwilson

@dougwilson From quick investigation - the first problem starts in the init function, it sets the wrong prototype - it's Http2Request/Response all the way until

    setPrototypeOf(req, app.request)
    setPrototypeOf(res, app.response)

Not sure how to proceed now :)

PiniH avatar Aug 07 '17 18:08 PiniH

Ah, the prototype. I guess that is different in the http2? Hmm, that sucks, because it is likely to be a large task to fix that. I would have assumed it wouldn't have been different, seeing as even the spdy module used those prototypes... and it's a third-party module that implemented http2 for a long time.

dougwilson avatar Aug 07 '17 18:08 dougwilson

Aye, it's a different constructor, I just started playing with it today, I will look into it more tomorrow and post my findings.

PiniH avatar Aug 07 '17 18:08 PiniH

I played with it some more, exposing Http2ServerRequest and Http2ServerResponse from node internal http2 files, and used them in the request/response files, (also had to define an empty setter for response.statusMessage - as Http2ServerResponse only defines a getter but no setter).

I managed to get a response (404, but still a response ;) ) - it seems to fail at matching the route -

 express:router dispatching GET /express +38s
  express:router query  : /express +1m
  express:router expressInit  : /express +34s

it seems to fail at a stack error (but only on the third call - not sure what's up with that yet) on

// get pathname of request
    var path = getPathname(req);

It got the correct path 2 times and then got stackoverflow. Any idea where/how to proceed?

PiniH avatar Aug 08 '17 09:08 PiniH

What's path returned as, and what's getPathname doing?

benjamingr avatar Aug 08 '17 09:08 benjamingr

From what I'm seeing, the getPathname is working properly, until the layer that's called expressInit is running (I guess that's an internal layer?), and then the getPathname fails with stackoverflow, will update soon with more details.

PiniH avatar Aug 08 '17 09:08 PiniH

Amusing...

Http2ServerRequest defines getter for 'url' property as return this.path, express request.js defines 'path' as

return parse(this).pathname;

which calls - you guessed it, request.url :) resulting in stack overflow, when commenting out the 'path' getter in request.js I got a response.

PiniH avatar Aug 08 '17 09:08 PiniH

any progress so far? is there workaround without touching http2(or express) core?

hansanghoon avatar Jan 23 '18 08:01 hansanghoon

Hi @hansanghoon all the work is happening in the linked PR: #3390

dougwilson avatar Jan 24 '18 04:01 dougwilson

I am still seeing this issue with Node v10.14.1 and [email protected]. Is there any timeline on resolving this problem?

glepsky avatar Dec 10 '18 15:12 glepsky

I am still seeing this issue with Node v10.14.1 and [email protected]. Is there any timeline on resolving this problem?

Same here...

ghost avatar Feb 21 '19 00:02 ghost

I got the same error today. Any updates?

aprilmintacpineda avatar Jan 04 '20 02:01 aprilmintacpineda

Same here, trying to serve up a react app over http/2 using the native http module from node.

I am using "express": "4.17.1" on Node v12.13.0

Any ideas ?

kiily avatar Jan 06 '20 00:01 kiily

is this being looked at? This means in-built node http2 module cannot be used for Express?

abhinavsingi avatar Jan 21 '20 07:01 abhinavsingi

https://github.com/expressjs/express/pull/3730

sdavids avatar Apr 07 '20 13:04 sdavids

Still not working

MatsG23 avatar May 14 '20 10:05 MatsG23

+1, not working

ycjcl868 avatar Jun 01 '20 11:06 ycjcl868

Not working on Node 14.5.0, express 4.17.1

calangoni avatar Jul 22 '20 09:07 calangoni

For now, http2 is not working with the latest version of Express. You can use spdy with Express, or just wait for Express v5.

see https://medium.com/@azatmardan/http-2-with-node-js-c928b90423d0

fuweichin avatar Feb 05 '21 09:02 fuweichin

FYI spdy does not work with node >= v15.

therealgilles avatar Feb 08 '21 19:02 therealgilles

Express does seem to have an open PR for this in version 5.0. Until then, I have created a workaround as a package(https://www.npmjs.com/package/http2-express-bridge). It worked perfectly on my application.

If anyone wants to experiment with it, do check it out.

Tested on node-14.15.5, express- 4.17.1

rahulramesha avatar Mar 02 '21 18:03 rahulramesha

Express does seem to have an open PR for this in version 5.0.

@rahulramesha There's a PR here: https://github.com/expressjs/express/pull/3730

aravindvnair99 avatar May 26 '21 11:05 aravindvnair99

Originally posted by @therealgilles in #3388 (comment) FYI spdy does not work with node >= v15.

There's the issue here in spdy: https://github.com/spdy-http2/node-spdy/issues/380

lamweili avatar Sep 19 '21 16:09 lamweili