Skip to content
This repository

add autoDiscovery of ports to servers #1412

Closed
wants to merge 2 commits into from

11 participants

Bradley Meck Robert Mustacchi Felix Geisendörfer ry TJ Holowaychuk Ben Noordhuis Isaac Z. Schlueter Diogo Resende Charlie Robbins Liam R. Black Nathan Rajlich
Bradley Meck
bmeck commented July 28, 2011

Adds a {autoDiscovery: true} option to servers. This will prevent EADDRINUSE when listening on the same port by incrementing the port being bound/listened to.

IE:

server1=net.createServer()
server2=net.createServer({autoDiscovery: true})

server1.listen(8888)
server2.listen(8888)

server2 will end up listening on 8889

Robert Mustacchi

Can you describe some cases where this makes sense over just specifying port 0 and using whatever the OS gives you? This behavior seems odd to me.

Felix Geisendörfer
Collaborator

-1. Could be done in user land, and patch contains debugging statements.

Update: Looked at the wrong diff, debugging statement was removed. But still -1, this is doable in user land easy enough.

ry
ry commented July 29, 2011

Can we think of a better name than autoDiscovery? Like forceBind?

Please make this pass with --use-uv too.

TJ Holowaychuk

-1 why not ephemeral via 0 like @rmustacc mentioned?

Ben Noordhuis

-1 from me too. I can't think of a single use case where I want port+1 if port is taken. Also, as @felixge pointed out, this is something user land can easily do by itself.

Isaac Z. Schlueter
Collaborator

I'd really like this in some scenarios. It'd be good if the server could know what port it ended up listening on, though.

Diogo Resende

@ry: maybe stick ?

@isaacs: I'm with you. Without knowing what port was binded this is useless.

Bradley Meck
Charlie Robbins

@isaacs The port is known by the time the listening event is raised, so perhaps it could be an argument there?

@felixge @bnoordhuis We are currently doing this in userland (http://github.com/nodejitsu/haibu-carapace) and bringing it into core would make it clear to module developers (specifically C++ addons, such as the zeromq addon) that it is a technique they should be aware of. There are a lot of distributed scenarios where this is useful: it reduces the amount of configuration management in a graph of nodes because they can simply come up and notify the network of their location.

Felix Geisendörfer
Collaborator

Well, this sure would be useful, so I guess I'm more +0 at this point.

But if we do this, I would prefer that this feature is exposed by simply calling listen() without a port. This would automatically start trying to bind to port 1024 (the first non-privileged port) and continue until an available port is found. The listen callback would provide the assigned port as the first parameter.

Charlie Robbins

@felixge +1 to doing this automatically when no port is passed, but the explicit option is important for script wrapping scenarios like haibu-carapace.

TJ Holowaychuk

/me curious, what's wrong with just using 0?

Charlie Robbins

The reason we wrote haibu-carapace is for starting multiple scripts on the same machine without any changes to the script itself.

For example if the script is

  require('http').createServer(function (req, res) { /* ... */ }).listen(8000);
Robert Mustacchi

This should stay in userland. This should definitely not be done automatically when no port is passed. That is why you can pass 0 for a port -- because you don't care. Otherwise you have turned what is guaranteed to be a single listen call into n listen system calls. Adding that behavior is counter-intuitive. Saying I don't care about a port shouldn't become let's try every port.

Charlie Robbins

@rmustacc This would not be done automatically. One would have to set autoDiscovery (or forceBind as @ry suggested), so it's really just another API to expose behavior that already exists.

Robert Mustacchi

@indexzero The automatic part is referring to what you and felixge previously said about doing it if no port is passed and that being the preferred way to expose the behavior.

Regardless of how it's exposed, this still seems like a great example of the kind of behavior that can be in userland. There are innumerable strategies that one can come up for how to handle the case of what do I do when the port I want isn't available. Some want to uniformly increase. Others use service management frameworks built into systems to do it. What if instead of increasing the port number I want to decrease?

TJ Holowaychuk

I get the use-case now but I agree it's pretty ad-hoc considering you could just proxy the net.Server#listen() method and provide 0

Liam R. Black

+1 doing .listen() sounds like a great idea. Should doing .listen() return the port number it binds to or is that to synchronous?

Ben Noordhuis

Committers: let's have a count of hands, it's getting confusing.

-1 from me.

Diogo Resende

+1 for .listen()

Nathan Rajlich
Collaborator

-1 for all of it. Should stay in userland...

Isaac Z. Schlueter
Collaborator

I'm coming down to -0.

It's a little convenient, but unnecessary, and pretty easy to implement in userland if you really need it by listening to the "error" event and handling EADDRINUSE.

But it doesn't offend me in any deep way.

Charlie Robbins

+1 for the reasons I've mentioned.

Not sure if my vote counts for anything, or if this kind of forced vote is appropriate. I share these sentiments on the topic: http://groups.google.com/group/nodejs-dev/msg/cfbb6cded3c4fef5

Liam R. Black

We don't have to absolutely use this convention but I definitely feel node.js should throughly document and convey how to easily achieve this functionality.

I can't tell you how many packages I download and run node server.js only to find the port is taken. Then you have to open up source having to isolate the listen(port) statement and the console.log statement. What is even worse is when the console.log statement is off from the listen statement, no error, no site displayed. what in the world?

Felix Geisendörfer
Collaborator

I'm +0.

Ben Noordhuis

@bmeck Thanks for the effort but we're not going to merge it.

Ben Noordhuis bnoordhuis closed this August 15, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
6  doc/api/net.markdown
Source Rendered
@@ -11,7 +11,8 @@ automatically set as a listener for the `'connection'` event.
11 11
 
12 12
 `options` is an object with the following defaults:
13 13
 
14  
-    { allowHalfOpen: false
  14
+    { allowHalfOpen: false,
  15
+      autoDiscovery: false
15 16
     }
16 17
 
17 18
 If `allowHalfOpen` is `true`, then the socket won't automatically send FIN
@@ -19,6 +20,9 @@ packet when the other end of the socket sends a FIN packet. The socket becomes
19 20
 non-readable, but still writable. You should call the end() method explicitly.
20 21
 See `'end'` event for more information.
21 22
 
  23
+If `autoDiscovery` is `true`, then the socket will bind to the closest
  24
+available address when listening rather than throwing an EADDRINUSE error.
  25
+
22 26
 ### net.createConnection(arguments...)
23 27
 
24 28
 Construct a new socket object and opens a socket to the given location. When
37  lib/net.js
@@ -898,6 +898,7 @@ function Server(/* [ options, ] listener */) {
898 898
   self.connections = 0;
899 899
 
900 900
   self.allowHalfOpen = options.allowHalfOpen || false;
  901
+  self.autoDiscovery = options.autoDiscovery || false;
901 902
 
902 903
   self.watcher = new IOWatcher();
903 904
   self.watcher.host = self;
@@ -1033,11 +1034,9 @@ Server.prototype.listen = function() {
1033 1034
     // Don't bind(). OS will assign a port with INADDR_ANY.
1034 1035
     // The port can be found with server.address()
1035 1036
     self.type = 'tcp4';
1036  
-    self.fd = socket(self.type);
1037 1037
     self._doListen(port);
1038 1038
   } else if (port === false) {
1039 1039
     // the first argument specifies a path
1040  
-    self.fd = socket('unix');
1041 1040
     self.type = 'unix';
1042 1041
     var path = arguments[0];
1043 1042
     self.path = path;
@@ -1067,7 +1066,6 @@ Server.prototype.listen = function() {
1067 1066
         self.emit('error', err);
1068 1067
       } else {
1069 1068
         self.type = addressType == 4 ? 'tcp4' : 'tcp6';
1070  
-        self.fd = socket(self.type);
1071 1069
         self._doListen(port, ip);
1072 1070
       }
1073 1071
     });
@@ -1092,16 +1090,28 @@ Server.prototype._startWatcher = function() {
1092 1090
 
1093 1091
 Server.prototype._doListen = function() {
1094 1092
   var self = this;
  1093
+  self.fd = socket(self.type);
1095 1094
 
1096 1095
   // Ensure we have a dummy fd for EMFILE conditions.
1097 1096
   getDummyFD();
1098 1097
 
1099  
-  try {
1100  
-    bind(self.fd, arguments[0], arguments[1]);
1101  
-  } catch (err) {
1102  
-    self.close();
1103  
-    self.emit('error', err);
1104  
-    return;
  1098
+  var port = arguments[0];
  1099
+  var ip = arguments[1];
  1100
+  
  1101
+  for(;;) {
  1102
+    try {
  1103
+      bind(self.fd, port, ip);
  1104
+      break;
  1105
+    } catch (err) {
  1106
+      if (self.autoDiscovery && err.code === 'EADDRINUSE') {
  1107
+        port++;
  1108
+      }
  1109
+      else {
  1110
+        self.close();
  1111
+        self.emit('error', err);
  1112
+        return;
  1113
+      }
  1114
+    }
1105 1115
   }
1106 1116
 
1107 1117
   // Need to the listening in the nextTick so that people potentially have
@@ -1115,8 +1125,13 @@ Server.prototype._doListen = function() {
1115 1125
     try {
1116 1126
       listen(self.fd, self._backlog || 128);
1117 1127
     } catch (err) {
1118  
-      self.close();
1119  
-      self.emit('error', err);
  1128
+      if (self.autoDiscovery && err.code === 'EADDRINUSE') {
  1129
+        self._doListen(port+1, ip);
  1130
+      }
  1131
+      else {
  1132
+        self.close();
  1133
+        self.emit('error', err);
  1134
+      }
1120 1135
       return;
1121 1136
     }
1122 1137
 
44  test/simple/test-net-autodiscovery.js
... ...
@@ -0,0 +1,44 @@
  1
+var common = require('../common');
  2
+var assert = require('assert');
  3
+var net = require('net');
  4
+
  5
+server1 = net.createServer();
  6
+server2 = net.createServer({autoDiscovery: true});
  7
+
  8
+var i = 0;
  9
+function check() {
  10
+  i++;
  11
+  if (i != 2) {
  12
+    return;
  13
+  }
  14
+  
  15
+  var addr1 = server1.address();
  16
+  var addr2 = server2.address();
  17
+  
  18
+  server1.close();
  19
+  server2.close();
  20
+  
  21
+  assert.equal(addr1.port, common.PORT);
  22
+  assert.notEqual(addr2, null);
  23
+  assert.notEqual(addr2.port, common.PORT);
  24
+  
  25
+}
  26
+
  27
+server1.on('listening', check);
  28
+server2.on('listening', check);
  29
+
  30
+server1.on('error', function(err) {
  31
+  assert.ifError(err);
  32
+  
  33
+  if(server1.fd) server1.close();
  34
+  if(server2.fd) server2.close();
  35
+});
  36
+server2.on('error', function(err) {
  37
+  assert.ifError(err);
  38
+  
  39
+  if(server1.fd) server1.close();
  40
+  if(server2.fd) server2.close();
  41
+});
  42
+
  43
+server1.listen(common.PORT);
  44
+server2.listen(common.PORT);
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.