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

NodeJS listener bug #8051

Closed
adrian-nitu-92 opened this issue Aug 10, 2016 · 17 comments
Closed

NodeJS listener bug #8051

adrian-nitu-92 opened this issue Aug 10, 2016 · 17 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter. net Issues and PRs related to the net subsystem.

Comments

@adrian-nitu-92
Copy link
Contributor

adrian-nitu-92 commented Aug 10, 2016

  • Version: all after b7a8a69
  • Platform: Linux automation-tck 3.19.0-25-generic GitHub issue management #26~14.04.1-Ubuntu SMP Fri Jul 24 21:16:20 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: core

Commit at: b7a8a69 contains a bug that prevents node from listening on a port.

I have detected this bug while trying to run acmeair-nodejs ( https://github.com/acmeair/acmeair-nodejs ) on the latest version of node.

Netstat report:

../builds/latest/node/node app.js &
[1] 130509
netstat -l | grep :9080
fg
../builds/latest/node/node app.js
^C

Expected report:

~/baseline/node/install/usr/local/bin/node app.js &
[1] 130519
netstat -l | grep :9080
tcp6       0      0 [::]:9080               [::]:*                  LISTEN     
fg
~/baseline/node/install/usr/local/bin/node app.js
^C

I am sure this is the commit with the issue because the commit 1b99093 is a_ok (manually tested).

@adrian-nitu-92
Copy link
Contributor Author

Oups :), missclick

@addaleax
Copy link
Member

Can you try to reduce this to a small reproducible test case?

@addaleax addaleax added events Issues and PRs related to the events subsystem / EventEmitter. net Issues and PRs related to the net subsystem. labels Aug 10, 2016
@adrian-nitu-92
Copy link
Contributor Author

Looking into it :D

@jasnell
Copy link
Member

jasnell commented Aug 10, 2016

FWIW, A simple test case with an http server running os x with master isn't showing any issues, so a reproducible test case would be very helpful :-)

@addaleax addaleax added this to the 7.0.0 milestone Aug 11, 2016
@addaleax
Copy link
Member

Added the 7.0.0 milestone here because the commit that might be causing it is a semver-major one that would be included in v7, just so we don’t forget about this issue.

@gareth-ellis
Copy link
Member

gareth-ellis commented Aug 12, 2016

That seems genuine:

https://benchmarking.nodejs.org/charts/acmeair_throughput.png

The last three days we've not been able to collect throughput on acmeair in the benchmarking workgroup - see the three blue dots at the bottom!

Edit: that said, I can't recreate it on my machine here....I'm trying to get a copy of the logs from our benchmarking run to see if there's anything that sticks out

@adrian-nitu-92
Copy link
Contributor Author

adrian-nitu-92 commented Aug 12, 2016

Helllo, i'm still looking into the issue and what caused it.
I managed to solve the issue by re-cloning acmeair and doing a npm install. The clone seems identical to my version of acmeair (_I checked only files tracked by git_) but maybe a secret (deep) dependency in node_modules breaks when it has to "cooperate" with this commit.
I am still trying to see exactly what happened but the walk over the dependency tree is really expensive :)

@gareth-ellis
Copy link
Member

Can you do npm ls and find the versions of modules ? - this could perhaps be it breaks on an old copy of express....

@adrian-nitu-92
Copy link
Contributor Author

Oups, seems I made a mistake on my previous post. It did not actually work(oups, I also have a stable version of node installed :D) and I was going mad trying to find the issue.

The issue happens in mongodb.connect. Sample code that doesn't work:

var mongodb = require('mongodb');

var mongourl = "mongodb://localhost:27017/test";

var c_opt = {server:{auto_reconnect:true,poolSize: 10}};
mongodb.connect(mongourl, c_opt, function(err, conn){
         console.error("fixed!");
    });

Acmeair doesn't work because it first wants the connection to the DB established and then it wants to actually start listening.

@gareth-ellis
Copy link
Member

Mongo db seems to use timer in quite a lot of places - however upgrading mongo to 2.0.x seems to work fine for both the simple mongo test and acme air.

@adrian-nitu-92
Copy link
Contributor Author

I can fully confirm that using mongo 2.0.x is a working fix.

I can continue our tests with the new change, so as far as I'm concerned, the issue is resolved.

  1. Should I try to make a pull request to acmeair-nodejs with this version of mongo(1.4.x->2.0.x)?
  2. I see the benchmarking group is still reporting no results for acmeair, @gareth-ellis, can you give them a nudge :D?

@gareth-ellis
Copy link
Member

I'm in the workgroup, I'll get the copy of acmeair updated shortly.

However, I still think we should identify which bit of mongo fails on the the latest, and convince ourselves that the change in Node is correct - there could well be a number of other modules that are also broken, but no one has tested on the master branch.....

@jasnell ,what are your thoughts on this?

@jasnell
Copy link
Member

jasnell commented Aug 22, 2016

Additional testing is always good. I'm still pretty certain the change in
core was the correct thing to do but it would be good to see if anything
else was affected this way.

On Monday, August 22, 2016, Gareth Ellis notifications@github.com wrote:

I'm in the workgroup, I'll get the copy of acmeair updated shortly.

However, I still think we should identify which bit of mongo fails on the
the latest, and convince ourselves that the change in Node is correct -
there could well be a number of other modules that are also broken, but no
one has tested on the master branch.....

@jasnell https://github.com/jasnell ,what are your thoughts on this?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8051 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2ebHm4bFm_EPijGxmUkmq5KprVyWdks5qiX8AgaJpZM4JhL8K
.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 5, 2016

Added the 7.0.0 milestone here because the commit that might be causing it is a semver-major one that would be included in v7, just so we don’t forget about this issue.

@addaleax you tagged this for v7, mind to check it again? :D

@addaleax
Copy link
Member

addaleax commented Oct 5, 2016

Mhm, I wasn’t too successful getting the mongodb@1.4.x test suite up and running. So, unless @adrian-nitu-92 or @gareth-ellis can give any further clues here, there might be nothing we can do.

@gareth-ellis
Copy link
Member

I also haven't been successful in narrowing down what in MongoDB 1.4.x stops working but works in 2.0.x.

@Fishrock123 Fishrock123 removed this from the 7.0.0 milestone Oct 6, 2016
@Trott
Copy link
Member

Trott commented Jul 10, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants