Skip to content
This repository has been archived by the owner. It is now read-only.

socket.on('data') event never fires due to node internals hijacking socket.ondata #7088

Closed
defunctzombie opened this issue Feb 9, 2014 · 4 comments

Comments

@defunctzombie
Copy link

@defunctzombie defunctzombie commented Feb 9, 2014

The following simple http server shows the failure and expected behavior. If you run this file and open localhost:5000 the expected behavior would be to exit the process, however this does not happen because the http fastpath in node does not use the emitter to capture data events (for performance).

var http = require('http');

var server = http.createServer(function(req, res) {});

server.on('connection', function(socket) {
    socket.on('data', function(chunk) {
        console.log('yay!');
        process.exit(0);
    });
});

server.listen(5000);

This issue is very difficult to debug if you are not aware of the node behavior/internals. I think that we need to either document this or even better would be to identify if there are data events and firing them accordingly. Care should be taken to identify if there are no data events after a removeListeners call too.

This is an issue on latest (0.10.25) node.js stable

/cc @guille @nullbox

@indutny
Copy link
Member

@indutny indutny commented Feb 9, 2014

This is well-known, and is fixed in v0.11.

@indutny indutny closed this Feb 9, 2014
@defunctzombie
Copy link
Author

@defunctzombie defunctzombie commented Feb 9, 2014

@indutny Can we get this backported to 0.10?

@defunctzombie
Copy link
Author

@defunctzombie defunctzombie commented Feb 9, 2014

@indutny I will add that I don't think that this issues is "well known" at all (certainly not for those that are unfamiliar with node internals) and certainly something that I would perceive as a bug in current behavior (thus asking for backporting).

@indutny
Copy link
Member

@indutny indutny commented Feb 12, 2014

@defunctzombie I'm afraid that this will come with some serious performance regressions, which couldn't be mitigated without backporting a serious portions of v0.11 code and probably breaking a backward compatibility in a few places. Sorry.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants