node http code change could have major effect on express

[WORMSS]

In node, commit nodejs/node@a899576c that landed yesterday can have major problems for express if someone does use the IncomingMessage option.

On line 31 of request https://github.com/expressjs/express/blob/master/lib/request.js#L31 express references http.IncomingMessage.prototype to extend it with all the express functionality.

If someone changes the http IncomingMessage class and doesn't extend IncomingMessage then the reqs will not have any of express functions.

Not sure why someone would, but they have the option to replicate the whole class without having to extend the real one.

I haven't tested this yet (I will need to learn how to build node).. but I wanted to give everyone the heads up.

[dougwilson]

Very interesting. I wonder if there is a change we can make to Express to compensate for this (ideally backwards compatible with older Node.js versions and folks' existing apps)? I wonder if Node.js core is aware of this; Express.js is still part of the Node.js foundation after all :D

[wesleytodd]

I really think we need to remove the prototypical inheritance here. I think this should be a requirement for 5.x. Not only for this issue, but because replacing the prototype is a perf issue (and one of the reasons that my new employer does not use express, along with the recursion in the router).

I started a PR to at least extract the current method into separate modules, but I really think it needs to go a step further. With the relative stability of these api's, I think the maintenance cost of wrapping them with our own req/res will not be that much.

I am at work right now so can't spend much time writing this up, but I have a bunch of ideas for this.

[dougwilson]

along with the recursion in the router

What recursion on the router? Wasn't that removed many releases ago to address that Netflix article? Definitely open an issue regarding this so it can be addressed / at least be known about :) @wesleytodd you've even an Express member now, lol

I am at work right now so can't spend much time writing this up, but I have a bunch of ideas for this.

Definitely please share :) ! Express 5.x is our opening to make significant changes. If you want to get together to brainstorm some as well I'm open to scheduling something.

[dougwilson]

What I'm thinking of is that article https://medium.com/netflix-techblog/node-js-in-flames-ddd073803aa4 which was fixed in Express 4.11.0 Jan 2015.

[wesleytodd]

Yeah, that's the article I am referencing, and afaik we are much better off than we were, but restify takes a different approach which has pros and cons. In a meeting right now, but I would like to write down all of the things I have been hearing from people here at Netflix which could be help to us improving and pushing express forward :)

[dougwilson]

Awesome, @wesleytodd looking forward to that. Didn't know you worked at Netflix now, congrats! We have the https://github.com/expressjs/discussions repo to have a place for over-arching planning discussions, especially useful for items that will span multiple repos.

[dougwilson]

replacing the prototype is a perf issue

Though this was done before I was on Express, ultimately this is a better approach than Restify, which actually globally patches Node.js internal objects (https://github.com/restify/node-restify/blob/master/lib/server.js#L33-L34) affecting all http servers running in the same process. If I had to choose between the way Express currently adds sugar and how Restify currently adds sugar, I would say Express does this is a better way. They are both bad, don't get we wrong, just comparing imo Express is the least bad currently, haha.

[wesleytodd]

Yeah, I agree the way restify is doing it is not good either. Actually I think restify is doing a ton of things much worse than express. But there are some performance things which, AFAIK, they can prove they do better. My hope is we can find a better middle ground that has the implementation strengths from express, but the perf optimizations which Netflix relies on.

I just reached out to the author of that article, he is still at Netflix, to see if I can pick his brain. So lets see if I can make that happen.

[dougwilson]

Gotcha. Yea, Express is open to making the improvements necessary. The biggest blocker is even knowing about them to do anything. This is a common barrier between corporate and the open source world (I'm in corporate world too, so defiantly know): internal to the corporation employees will do many things with open source software, be that finding bugs, quirks, evaluating different solutions with each other. But ultimately none of the findings are ever spoken about outside of the corporation, or at best these days there is a too-generic engineering PR post made on the subject.

Performance comparisons are even harder, because often times the employees tasks with doing that work just don't know the ins-and-outs of what they are testing (maybe knows just one really well). I have personally overseen a team doing performance testing that testing two pieces much lower than the others, because they made a coding mistake since they didn't read any documentation and just Googled to quickly slap code together :) I'm not saying this is how they are all, but it's hard to known even when summaries are posted because they usually don't include a method to reproduce the benchmarks.

[wesleytodd]

So it turns out that the original author of that PR to node core is a team member here at Netflix, and it was particularly so that Restify did not need to klober the core req/res. @dougwilson I am going to work on writing up a few things looking at the differences between restify and express, and a proposal for how we could collaborate and combine efforts. I will post it up in the discussions repo and link back here when I make progress on that.

[dougwilson]

Nice, looking forward to it :+1: looking at the commit I don't think it's possible for express to make use of it, because Express is designed to just be handed a req and res object which allows Express the flexibility to be embedded within Hapi servers, Koa servers, Connect servers, etc. That commit looks useful for frameworks that do not allow this and instead create the underlying HTTP server like Hapi and Restify, for example.