Skip to content
This repository

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

Closed
ry opened this Issue · 18 comments

6 participants

ry Arlo Breault Felix Geisendörfer Jérémy Lal Koichi Kobayashi Ben Noordhuis
ry
ry commented

I think because the setTimeouts do not guarantee order

Arlo Breault arlolra referenced this issue from a commit
Arlo Breault arlolra Fixes #1333 6e0faf4
Arlo Breault

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.

Felix Geisendörfer
Collaborator

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?

Jérémy Lal

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

Jérémy Lal

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.

Koichi Kobayashi
Collaborator

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?

Arlo Breault

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

Koichi Kobayashi
Collaborator

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

Arlo Breault

@koichik i did arlolra/node@b6ceb02

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

Koichi Kobayashi
Collaborator

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

Arlo Breault

@koichik riiight

it did show up here though
#1347

Koichi Kobayashi
Collaborator

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

Arlo Breault

Not sure I follow :)

Koichi Kobayashi
Collaborator

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

Arlo Breault

@koichik no problem

Koichi Kobayashi
Collaborator

@arlolra - Thanks!

@ry @felixge @bnoordhuis - Can you review koichik/node@acd5583?

Koichi Kobayashi
Collaborator

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.

Koichi Kobayashi
Collaborator

@bnoordhuis - Thanks, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.