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

req & res in http.createServer have not the same map #6294

Closed
welefen opened this issue Apr 20, 2016 · 6 comments
Closed

req & res in http.createServer have not the same map #6294

welefen opened this issue Apr 20, 2016 · 6 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@welefen
Copy link

welefen commented Apr 20, 2016

  • Version: v4.4.3
  • Platform: Mac os
  • Subsystem:
var http = require('http');

var prevReq = null;
var prevRes = null;
var server = http.createServer(function(req, res){
  if(!prevReq){
    prevReq = req;
    prevRes = res;
  }else{
    console.log('req', %HaveSameMap(req, prevReq));
    console.log('res', %HaveSameMap(res, prevRes));
  }
  res.end('hello word');
});

server.listen(3337);

console.log('server running at http://127.0.0.1:3337');

use node --allow-natives-syntax index.js to test.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Apr 20, 2016
@evanlucas
Copy link
Contributor

From what I've seen, the _idlePrev and _idleNext properties on both req and res can have different maps.

@welefen
Copy link
Author

welefen commented Apr 20, 2016

https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L71 , This Line add dynamic property for IncomingMessage, It caused IncomingMessage instance have not the same map.

@Fishrock123
Copy link
Contributor

I don't understand how this is a problem...

Closing, let us know if we need to re-open.

@bnoordhuis
Copy link
Member

@Fishrock123 It means request and response objects are implemented much less efficiently than they can be. The issue is legitimate, I'll reopen (and try to look at it this week.)

@bnoordhuis bnoordhuis reopened this May 25, 2016
@welefen
Copy link
Author

welefen commented May 25, 2016

Request and Reponse object in createServer has not same map, So it can not use Hidden Classes optimizing in V8, Hidden Classes is about 2x faster when get class instance. https://developers.google.com/v8/design#fast-property-access

@bnoordhuis
Copy link
Member

I had a look and I filed #7003 as a basic sanity check.

It doesn't catch this particular issue but I've come to believe the test case is something of a red herring because it's comparing objects at different phases in their life cycle. If you rewrite the test to only check at the end (like I did in #7003), everything is fine.

That's not to say the http module is currently a paragon of efficiency - it's not - but this particular issue doesn't seem valid. I'll close it for now.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jun 29, 2016
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

PR-URL: nodejs#7003
Refs: nodejs#6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants