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

Support for module http2? #2761

Open
jabrena opened this issue Sep 23, 2015 · 26 comments

Comments

Projects
None yet
@jabrena
Copy link

commented Sep 23, 2015

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:
molnarg/node-http2#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

This comment has been minimized.

Copy link
Author

commented Sep 23, 2015

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 dougwilson self-assigned this Sep 23, 2015

@dougwilson

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

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 molnarg/node-http2#100 (comment)) 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 dougwilson closed this Sep 23, 2015

@dougwilson

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

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 dougwilson reopened this Sep 23, 2015

@dougwilson dougwilson added 5.x and removed 4.x question labels Sep 23, 2015

@jabrena

This comment has been minimized.

Copy link
Author

commented Sep 23, 2015

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

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

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

This comment has been minimized.

Copy link
Author

commented Sep 23, 2015

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:
molnarg/node-http2#100

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

Juan Antonio

@listepo

This comment has been minimized.

Copy link

commented Oct 10, 2015

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

With this module you can create HTTP2 / SPDY

@dougwilson

This comment has been minimized.

Copy link
Member

commented Oct 10, 2015

AFAIK node-spdy already works with Express 4

@listepo

This comment has been minimized.

Copy link

commented Oct 10, 2015

👍

@jabrena

This comment has been minimized.

Copy link
Author

commented Oct 10, 2015

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

@ronkorving

This comment has been minimized.

Copy link

commented Oct 23, 2015

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2016

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

@dougwilson

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

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

This comment has been minimized.

Copy link

commented Feb 26, 2016

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

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

This comment has been minimized.

Copy link

commented Feb 26, 2016

You mean like #2781 ? :)

@dougwilson

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

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

This comment has been minimized.

Copy link

commented Feb 26, 2016

No problem 👍

@cchamberlain

This comment has been minimized.

Copy link

commented May 3, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

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

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

OK, so the main problem I found with #2761 (comment) 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

This comment has been minimized.

Copy link
Contributor

commented May 25, 2016

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

@ericmdantas ericmdantas referenced this issue Aug 16, 2016

Closed

Implement http2 server #96

6 of 6 tasks complete

@ericmdantas ericmdantas referenced this issue Oct 26, 2016

Closed

http/2 #25

zaknuces added a commit to zaknuces/oz-vox-server that referenced this issue Mar 18, 2017

[OZ-0001] skeleton code for express http2 server
Refer:
expressjs/express#2761 (comment)
Need to revisit setting request response prototypes statically.

@goliatone goliatone referenced this issue May 1, 2017

Open

Support TLS #33

@p3x-robot

This comment was marked as off-topic.

Copy link

commented Nov 14, 2018

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

This comment has been minimized.

Copy link

commented Feb 22, 2019

Is http/2 still on roadmap?

@asbjornu

This comment has been minimized.

Copy link

commented Mar 24, 2019

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

@krzysdz

This comment has been minimized.

Copy link

commented Mar 25, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.