test/simple/test-net-server-max-connections.js is racey #1333

Closed
ry opened this Issue Jul 14, 2011 · 18 comments

6 participants

@ry
ry commented Jul 14, 2011

I think because the setTimeouts do not guarantee order

@arlolra arlolra added a commit that referenced this issue Jul 15, 2011
@arlolra arlolra Fixes #1333 6e0faf4
@arlolra

Maybe my assumption is wrong, but you don't need to index the connections. Only make sure that if a connection is refused (ie. closes without getting data) the server has already reached its max connections.

I also took the liberty of doing the TODO.

@felixge

I think because the setTimeouts do not guarantee order

It does guarantee order for timeouts of the same value registered in the same tick right? Since you're queueing those into one array / ev_timer right?

@kapouer

A way to fix the race for this test has been reported to work, please read :
http://bugs.debian.org/643849#20
http://bugs.debian.org/643849#25

@kapouer

I just read this interesting piece at http://javascriptquiz.com/blog/q10-who-shot-first.html :

The reason for this is that according to the spec the minimum for a timeout in JavaScript is 4 milliseconds and any timeout that is less than 4 milliseconds will be increased to 4 milliseconds.

@koichik koichik added a commit to koichik/node that referenced this issue Nov 2, 2011
@koichik koichik test: fix test/simple/test-net-server-max-connections.js is racey
Fixes #1333.
acd5583
@koichik

Without indent, koichik/node@acd5583 changed only the following:

--- a/test/simple/test-net-server-max-connections.js
+++ b/test/simple/test-net-server-max-connections.js
@@ -39,12 +39,12 @@ var server = net.createServer(function(connection) {
   console.error('connect %d', count++);
   connection.write('hello');
   waits.push(function() { connection.end(); });
+  assert.ok(count <= N / 2);
+  makeConnection(count);
 });

 server.listen(common.PORT, function() {
-  for (var i = 0; i < N; i++) {
-    makeConnection(i);
-  }
+  makeConnection(0);
 });

 server.maxConnections = N / 2;
@@ -53,7 +53,6 @@ console.error('server.maxConnections = %d', server.maxConnecti


 function makeConnection(index) {
-  setTimeout(function() {
     var c = net.createConnection(common.PORT);
     var gotData = false;

@@ -77,6 +76,7 @@ function makeConnection(index) {
                   index +
                   ' was one of the first closed connections ' +
                   'but shouldnt have been');
+        makeConnection(++count);
       }

       if (closes === N / 2) {
@@ -96,7 +96,6 @@ function makeConnection(index) {
                      index + ' got data, but shouldn\'t have');
       }
     });
-  }, index);
 }

Can someone review?

@arlolra

LGTM but I thought what I had written was alright as well.

@koichik

@arlolra - If you had modified an indent, your patch would have been landed. :-)

@arlolra

@koichik i did arlolra/node@b6ceb02

not sure why github didn't add that, it was pushed to the same branch :(

@koichik

Oh... because this issue is not pull-request, the commit log should include the issue number...

@arlolra

@koichik riiight

it did show up here though
#1347

@koichik

Ooops, sorry, I cannot find #1347. because it does not have "max-connections"...

@arlolra

Not sure I follow :)

@koichik

@arlolra - Because my patch removes setTimeout(), I want to merge it. Is it OK?

@arlolra

@koichik no problem

@koichik koichik added a commit to koichik/node that referenced this issue Jan 22, 2012
@koichik koichik test: fix test/simple/test-net-server-max-connections.js is racey
Fixes #1333.
75109c8
@koichik

Updated by @bnoordhuis's suggestion.
Without indent, koichik/node@75109c8 changed only the following:

--- a/test/simple/test-net-server-max-connections.js
+++ b/test/simple/test-net-server-max-connections.js
@@ -42,9 +42,7 @@ var server = net.createServer(function(connection) {
 });

 server.listen(common.PORT, function() {
-  for (var i = 0; i < N; i++) {
-    makeConnection(i);
-  }
+  makeConnection(0);
 });

 server.maxConnections = N / 2;
@@ -53,10 +51,15 @@ console.error('server.maxConnections = %d', server.maxConnec


 function makeConnection(index) {
-  setTimeout(function() {
     var c = net.createConnection(common.PORT);
     var gotData = false;

+    c.on('connect', function() {
+      if (index + 1 < N) {
+        makeConnection(index + 1);
+      }
+    });
+
     c.on('end', function() { c.end(); });

     c.on('data', function(b) {
@@ -96,7 +99,6 @@ function makeConnection(index) {
                      index + ' got data, but shouldn\'t have');
       }
     });
-  }, index);
 }

Please review.

@bnoordhuis
Node.js Foundation member

@koichik: LGTM

@koichik

@bnoordhuis - Thanks, merging.

@koichik koichik closed this in 8271800 Jan 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment