express icon indicating copy to clipboard operation
express copied to clipboard

Support for module http2?

Open jabrena opened this issue 8 years ago • 35 comments

Hi, I would like to know if Express 5.0 will have support for http2: https://github.com/molnarg/node-http2

I was reading a bit and I noticed that exist a problem with http2 module: https://github.com/molnarg/node-http2/issues/100

I have tested in local and the problem continues: https://github.com/jabrena/CloudFoundryLab/blob/master/Node_HelloWorld_http2_express/index.js

It this issue in the roadmap? Does exist another alternative to run a express application with http2?

Many thanks in advance.

Juan Antonio

jabrena avatar Sep 23 '15 08:09 jabrena

I have just tested with latest release alpha (5.0.0-alpha.2) and the problem continues:


/*jslint node: true*/
"use strict";

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

var app = express();
app.get('/', function (req, res) {
    console.log(req.headers);

    res.header('Content-type', 'text/html');
    return res.end('<h1>Hello, Secure World!</h1>');
});

var options = {
    key: fs.readFileSync('./certificate/localhost.key'),
    cert: fs.readFileSync('./certificate/localhost.crt')
};
var port = process.env.VCAP_APP_PORT || 8080;
http2.createServer(options, app).listen(port);

traces:


    dest.end();
         ^

TypeError: dest.end is not a function
    at Stream.onend (_stream_readable.js:490:10)
    at Stream.g (events.js:260:16)
    at emitNone (events.js:72:20)
    at Stream.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:893:12)
    at doNTCallback2 (node.js:429:9)
    at process._tickCallback (node.js:343:17)
23 Sep 12:15:24 - [nodemon] app crashed - waiting for file changes before starti
ng...

jabrena avatar Sep 23 '15 10:09 jabrena

Hi! o answer your question, yes, it is a known issue, and no, has not yet been addressed. We have a link back to that referenced http2 issue (noted in https://github.com/molnarg/node-http2/issues/100#issuecomment-111549799) already to track this as well as it listed in our Express 5 roadmap (#2237):

Support non-core HTTP prototypes (e.g. http2, shot and others)

The 5.0 alpha 2 does not work because it has not been addressed yet. It is not a simple fix at all, and any help you want to provide toward this as part of a PR is much appreciated!

dougwilson avatar Sep 23 '15 14:09 dougwilson

In addition, if all you want to do is replicate the Express routing with http2, the module router (https://github.com/pillarjs/router) is the actual Express 4.x router, extracted into it's own module and will work just fine with http2:

/*jslint node: true*/
"use strict";

var finalhandler = require('finalhandler');
var fs = require('fs');
var http2 = require('http2');
var Router = require('router');

var router = new Router();
router.get('/', function (req, res) {
    console.log(req.headers);

    res.setHeader('Content-type', 'text/html');
    return res.end('<h1>Hello, Secure World!</h1>');
});

var options = {
    key: fs.readFileSync('./certificate/localhost.key'),
    cert: fs.readFileSync('./certificate/localhost.crt')
};
var port = process.env.VCAP_APP_PORT || 8080;
http2.createServer(options, app).listen(port);

function app(req, res) {
    router(req, res, finalhandler(req, res));
}

dougwilson avatar Sep 23 '15 14:09 dougwilson

Hi @dougwilson, if you provide some technical documentation to check that classes, I could help you to begin fixing this issue. Tomorrow, I will test the alternative.

jabrena avatar Sep 23 '15 15:09 jabrena

Hi @jabrena , I'm not sure what kind of technical documentation you are looking for. All Express documentation is located at http://expressjs.com/ and anything technical is gathered from reading the JavaScript source code of Express.

dougwilson avatar Sep 23 '15 15:09 dougwilson

No problem, will try to check this part:

Prototype = req.__proto__; 
req.__proto__ = app.request; app.request.__proto__ = originalPrototype; 

it is a clue from a comment in: https://github.com/molnarg/node-http2/issues/100

Besides, I will compare current router with the router that runs with http2

Juan Antonio

jabrena avatar Sep 23 '15 15:09 jabrena

@jabrena , @dougwilson why not https://github.com/indutny/node-spdy?

With this module you can create HTTP2 / SPDY

listepo avatar Oct 10 '15 09:10 listepo

AFAIK node-spdy already works with Express 4

dougwilson avatar Oct 10 '15 11:10 dougwilson

:+1:

listepo avatar Oct 10 '15 11:10 listepo

Pretty interesting!!! I will test next week. @victorherraiz @isuriv

jabrena avatar Oct 10 '15 16:10 jabrena

I can confirm node-spdy works fine, I'm using it. It doesn't however let you use HTTP/2 push promises very easily with Express. I made an issue about this at #2781.

ronkorving avatar Oct 23 '15 10:10 ronkorving

I think having http2 out of the box is perhaps a little out of scope of express itself?

Fishrock123 avatar Feb 19 '16 17:02 Fishrock123

I think having http2 out of the box is perhaps a little out of scope of express itself?

It may be, but I interpret this issue as the fact that it's impossible to even use the http2 module. Technically Express doesn't really do HTTP out of the box, unless you count the single app.listen entry point. HTTP support is, in the general sense, done with http.createServer(app), and that should work for the http2 and other modules for this issue to be considered closed.

dougwilson avatar Feb 26 '16 03:02 dougwilson

The problem to me is not that express is not compatible with the spdy module. They are compatible, it all works. However, if I want to use HTTP2 push promises, all the goodies of Express, like .static and sendFile become unusable. I cannot connect their logic to the push promise stream.

ronkorving avatar Feb 26 '16 03:02 ronkorving

Hi @ronkorving, perhaps open a new issue in regards to that problem? I don't want to loose sight on the issue brought up in the original post on the issue, which is specifically about working with http2. Please open a new issue, providing as much detail into the problem as you can, so we can reproduce and see what we can figure out.

dougwilson avatar Feb 26 '16 03:02 dougwilson

You mean like #2781 ? :)

ronkorving avatar Feb 26 '16 04:02 ronkorving

Ah, yep :) But I don't know anything about node-spdy and there is no code for me to even play with. Without a full way to reproduce and the technical details of the issue, I'm not sure how much help I can be on that issue. @jonathanong replied to you and he has used that module in the past, but I don't think anyone else has and I forgot that even existed. Can you fill out the issue some more for use non-spdy users?

dougwilson avatar Feb 26 '16 04:02 dougwilson

No problem :+1:

ronkorving avatar Feb 26 '16 05:02 ronkorving

@dougwilson - I went down your path of using router module and ran into issues with not being able to use res.sendFile anymore. I ended up extracting all the sendFile logic and reworking it as a thunk that works just like serveStatic (supports all sendFile options). I published it as serve-file module, may come in handy to anyone that ran into the same issues as me. I tested it on http2 / spdy with router and had no issues:

import serveFile from 'serve-file'

app.use('/my-file.txt', serveFile('path/to/my-file.txt'))

cchamberlain avatar May 03 '16 17:05 cchamberlain

I'm quite curious if initialising an express application as follows would be enough to resolve the problems. It seems to be enough to get a very simple application working (on express 5.0 branch):

var express = require('express');
var http2 = require('http2');

express.request.__proto__ = http2.IncomingMessage.prototype;
express.response.__proto__ = http2.ServerResponse.prototype;

var app = express();
...

tunniclm avatar May 04 '16 16:05 tunniclm

OK, so the main problem I found with https://github.com/expressjs/express/issues/2761#issuecomment-216912022 is that it doesn't handle HTTP/1.1 fallback properly. Since the prototype used here is statically set to the http2 prototype, it is incorrect when fallback happens to HTTP/1.1 resulting in a TypeError (attempt to call push() on undefined).

About a month ago, shortly before I posted the above comment I had implemented a change in my local express to dynamically fix-up the prototype chain and that's where I stumbled on express.request and express.response -- my patch broke that functionality.

However, I just ran the acme-air for Node.js benchmark app on my patched express and it seems to fix the problem with falling back. I'm going to see if I can solve the problem with breaking the express.request and express.response API, or make something that provides similar functionality and push a branch for wider testing.

tunniclm avatar May 24 '16 17:05 tunniclm

I've cleaned the patch up some and changed it a bit in light of some issues that I found during further testing -- I discovered app.request and app.response, the application level equivalents of express.request and express.response and changed the patch to preserve similar functionality.

The approach the patch takes is to resolve inheritance issues by mixing in the express API object instead of inserting it into the inheritance chain. Combined with lazily caching the prototype chain for each protocol that is used, a correct prototype chain can be kept for each request.

I the latest implementation the mixed-in API objects are also stored in the cache to avoid performing the mixin afresh on every request. This means there is a cache entry for each app and protocol combination in the system.

I have renamed the app-level and express-level request and response objects to make it clear they are now mixins. This may not be desirable as it will break code that modifies these API objects (for example, to add methods), however, it may be a good idea if the difference in behaviour would cause subtle bugs.

Since these mixins are performed once and cached thereafter, modifications to the API objects have no effect after the first request is handled (for a given app/protocol combination). This could be improved by providing a mechanism for dirtying the cache.

The branch (based on 5.0): https://github.com/tunniclm/express/tree/http2_with_dynamic_insertion2

Comparison with expressjs/express branch 5.0: https://github.com/expressjs/express/compare/5.0...tunniclm:http2_with_dynamic_insertion2

I have already tested a bit on a simple and a medium complexity express application, but not yet tried a server push (these were both converted HTTP/1.1 apps) -- I'll try that soon.

I'm not wedded to the approach taken in this patch, so feel free to point out if I'm going down the wrong track. I have several other things I'm trying, but this is the first one that is working at the moment.

I'd welcome anyone who can spare a little time to comment or try to test it out. Thanks!

tunniclm avatar May 25 '16 19:05 tunniclm

do you guys know when this will be working? koa has been working with http2 for a long time and now spdy is outdated as well as of node v11.1.0. we switched a while ago with spdy, but all upstream packages are outdated to work on node v11.1.0 :(

express is such a huge package, everyone knows it and still http2 is just not working. sad, but true.

p3x-robot avatar Nov 14 '18 09:11 p3x-robot

Is http/2 still on roadmap?

dhananjay1405 avatar Feb 22 '19 03:02 dhananjay1405

@tunniclm, your work looks promising. Any reason why you're not submitting it as a pull request towards the official repository?

asbjornu avatar Mar 24 '19 00:03 asbjornu

@asbjornu There's also PR #3730, which could add http/2 support for express and maybe it will be merged.

krzysdz avatar Mar 25 '19 22:03 krzysdz

For anyone saying just use spdy, HTTP/2 is an actual standard now. SPDY is not. On top of that spdy makes heavy use of private internal functions in node. It is broken and expected not to be fixed for any node version >= 11.1

IMO supporting the actual native http2 should be the current direction that express takes

SampsonCrowley avatar Jul 03 '19 06:07 SampsonCrowley

as @nwgh actually mentioned in https://github.com/molnarg/node-http2/issues/100 on Aug 9, 2016 already:

[...] just to note, spdy is dead.

So the SPDY package is no longer maintained, and due to a bug in the handle-thing dependency even broken when running with node > 11.1 (see https://github.com/nodejs/node/issues/24097). So going for SPDY is clearly no longer an option! The only way forward for express supporting HTTP/2 is the Node.js http2 package.

Is there any update on the progress about this? It'd be a bummer if Express 5 would not provide any HTTP/2 support! Thanks.

kristian avatar Oct 23 '19 21:10 kristian

Has anybody got anything to push to fix http/2 issues? log in #4112 for 4.17.1/5.0/5.x... we tried pillerjs/router as a fix but still leaves is in a broken state. many modules and process explicitly depend on express being available and have only managed to work through a fraction of them to remove express dependencies.

GaryMatthews avatar Dec 02 '19 05:12 GaryMatthews

fyi, the spdy/handle-thing issue has been fixed: https://github.com/spdy-http2/handle-thing/pull/13

I was just able to start an HTTP/2 server using spdy and express on Node v12.6.0 and v13.11.0. I get an ugly console message (node:48680) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated but apart from that, spdy seems to work with the latest Node.js versions again.

RandomByte avatar Mar 26 '20 22:03 RandomByte