Skip to content
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

Node http2 - cannot read this.socket.readable #3388

Open
PiniH opened this issue Aug 7, 2017 · 26 comments
Open

Node http2 - cannot read this.socket.readable #3388

PiniH opened this issue Aug 7, 2017 · 26 comments

Comments

@PiniH
Copy link

PiniH commented Aug 7, 2017

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)
@dougwilson
Copy link
Contributor

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 :) !

@PiniH
Copy link
Author

PiniH commented Aug 7, 2017

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

@dougwilson
Copy link
Contributor

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

@PiniH
Copy link
Author

PiniH commented Aug 7, 2017

@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 :)

@dougwilson
Copy link
Contributor

dougwilson commented Aug 7, 2017

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.

@PiniH
Copy link
Author

PiniH commented Aug 7, 2017 via email

@PiniH
Copy link
Author

PiniH commented Aug 8, 2017

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?

@benjamingr
Copy link

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

@PiniH
Copy link
Author

PiniH commented Aug 8, 2017

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
Copy link
Author

PiniH commented Aug 8, 2017

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.

@hansanghoon
Copy link

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

@dougwilson
Copy link
Contributor

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

@expressjs expressjs deleted a comment from danielgindi Jul 5, 2018
shesek added a commit to shesek/spark-wallet that referenced this issue Sep 1, 2018
This works, but it looks like the "spdy" module (which, despite its name, also supports http2)
is not compatible with newer nodejs versions and should be avoided:
spdy-http2/node-spdy#333
webpack/webpack-dev-server#1451

Should eventually use nodejs's native http2 module, but it is currently incompatible with express:
expressjs/express#3388

Committing the changes to a branch and setting aside for now.
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Oct 31, 2018
`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Oct 31, 2018
cherry pick of webpack@e97d345

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Oct 31, 2018
cherry pick of webpack@e97d345

This commit is in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Oct 31, 2018
cherry pick of webpack@e97d345

this issue was fixed is in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Oct 31, 2018
Ports fix for webpack#1449 to v2 branch
cherry pick of webpack@e97d345

this issue was fixed in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Nov 1, 2018
Ports fix for webpack#1449 to v2 branch
cherry pick of webpack@e97d345

this issue was fixed in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Nov 1, 2018
Ports fix for webpack#1449 to v2 branch
cherry pick of webpack@e97d345

this issue was fixed in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Nov 1, 2018
Ports fix for webpack#1449 to v2 branch
cherry pick of webpack@e97d345

this issue was fixed in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
RohanBhanderi pushed a commit to RohanBhanderi/webpack-dev-server that referenced this issue Nov 1, 2018
Ports fix for webpack#1449 to v2 branch
cherry pick of webpack@e97d345

this issue was fixed in webpack-dev-server v3 line, which requires webpack to be
upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3)
which does not require webpack to be upgraded to 4.0.0

`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

related issues:
nodejs/node#21665
nodejs/node#21665
webpack#1449
expressjs/express#3388
@glepsky

This comment has been minimized.

@ghost

This comment has been minimized.

@aprilmintacpineda
Copy link

I got the same error today. Any updates?

@kiily
Copy link

kiily commented Jan 6, 2020

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 ?

@abhinavsingi
Copy link

abhinavsingi commented Jan 21, 2020

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

@sdavids
Copy link

sdavids commented Apr 7, 2020

#3730

@MatsG23
Copy link

MatsG23 commented May 14, 2020

Still not working

@ycjcl868
Copy link

ycjcl868 commented Jun 1, 2020

+1, not working

@calangoni
Copy link

Not working on Node 14.5.0, express 4.17.1

@fuweichin
Copy link

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

@therealgilles
Copy link

FYI spdy does not work with node >= v15.

@rahulramesha
Copy link

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

@aravindvnair99
Copy link
Member

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

@rahulramesha There's a PR here: #3730

@lamweili
Copy link

lamweili commented Sep 19, 2021

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

There's the issue here in spdy: spdy-http2/node-spdy#380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests