Support for module http2?

jabrena commented 9 years ago

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 commented 9 years ago

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...

dougwilson commented 9 years ago

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 commented 9 years ago

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));
}

jabrena commented 9 years ago

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.

dougwilson commented 9 years ago

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.

jabrena commented 9 years ago

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

listepo commented 9 years ago

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

With this module you can create HTTP2 / SPDY

dougwilson commented 9 years ago

AFAIK node-spdy already works with Express 4

listepo commented 9 years ago

:+1:

jabrena commented 9 years ago

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

ronkorving commented 9 years ago

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.

Fishrock123 commented 8 years ago

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

dougwilson commented 8 years ago

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.

ronkorving commented 8 years ago

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.

dougwilson commented 8 years ago

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.

ronkorving commented 8 years ago

You mean like #2781 ? :)

dougwilson commented 8 years ago

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?

ronkorving commented 8 years ago

No problem :+1:

cchamberlain commented 8 years ago

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

tunniclm commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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!

p3x-robot commented 6 years ago

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.

dhananjay1405 commented 5 years ago

Is http/2 still on roadmap?

asbjornu commented 5 years ago

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

krzysdz commented 5 years ago

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

SampsonCrowley commented 5 years ago

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

kristian commented 5 years ago

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.

GaryMatthews commented 4 years ago

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.

RandomByte commented 4 years ago

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.