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

add autoDiscovery of ports to servers #1412

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@bmeck
Member

bmeck commented Jul 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

@rmustacc

This comment has been minimized.

Show comment
Hide comment
@rmustacc

rmustacc Jul 29, 2011

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.

rmustacc commented Jul 29, 2011

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.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 29, 2011

-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.

felixge commented Jul 29, 2011

-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

This comment has been minimized.

Show comment
Hide comment
@ry

ry Jul 30, 2011

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

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

ry commented Jul 30, 2011

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

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

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jul 30, 2011

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

tj commented Jul 30, 2011

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jul 30, 2011

Member

-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.

Member

bnoordhuis commented Jul 30, 2011

-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.

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs Aug 9, 2011

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.

isaacs commented Aug 9, 2011

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.

@dresende

This comment has been minimized.

Show comment
Hide comment
@dresende

dresende Aug 10, 2011

@ry: maybe stick ?

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

dresende commented Aug 10, 2011

@ry: maybe stick ?

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

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Aug 10, 2011

Member

Using server.address().port can get you the actual bound port for a
server easily.

On Wed, Aug 10, 2011 at 1:19 PM, dresende
reply@reply.github.com
wrote:

@ry: maybe stick ?

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

Reply to this email directly or view it on GitHub:
#1412 (comment)

Member

bmeck commented Aug 10, 2011

Using server.address().port can get you the actual bound port for a
server easily.

On Wed, Aug 10, 2011 at 1:19 PM, dresende
reply@reply.github.com
wrote:

@ry: maybe stick ?

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

Reply to this email directly or view it on GitHub:
#1412 (comment)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 10, 2011

@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.

indexzero commented Aug 10, 2011

@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.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Aug 11, 2011

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.

felixge commented Aug 11, 2011

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.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 11, 2011

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

indexzero commented Aug 11, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@tj

tj Aug 11, 2011

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

tj commented Aug 11, 2011

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

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 11, 2011

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);

indexzero commented Aug 11, 2011

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);
@rmustacc

This comment has been minimized.

Show comment
Hide comment
@rmustacc

rmustacc Aug 11, 2011

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.

rmustacc commented Aug 11, 2011

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.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 11, 2011

@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.

indexzero commented Aug 11, 2011

@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.

@rmustacc

This comment has been minimized.

Show comment
Hide comment
@rmustacc

rmustacc Aug 11, 2011

@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?

rmustacc commented Aug 11, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@tj

tj Aug 11, 2011

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

tj commented Aug 11, 2011

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

@limeb

This comment has been minimized.

Show comment
Hide comment
@limeb

limeb Aug 11, 2011

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

limeb commented Aug 11, 2011

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 11, 2011

Member

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

-1 from me.

Member

bnoordhuis commented Aug 11, 2011

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

-1 from me.

@dresende

This comment has been minimized.

Show comment
Hide comment
@dresende

dresende Aug 11, 2011

+1 for .listen()

dresende commented Aug 11, 2011

+1 for .listen()

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Aug 11, 2011

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

TooTallNate commented Aug 11, 2011

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

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs Aug 11, 2011

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.

isaacs commented Aug 11, 2011

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.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 11, 2011

+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

indexzero commented Aug 11, 2011

+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

@limeb

This comment has been minimized.

Show comment
Hide comment
@limeb

limeb Aug 11, 2011

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?

limeb commented Aug 11, 2011

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?

@tj

This comment has been minimized.

Show comment
Hide comment

tj commented Aug 11, 2011

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge commented Aug 15, 2011

I'm +0.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 15, 2011

Member

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

Member

bnoordhuis commented Aug 15, 2011

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

@bnoordhuis bnoordhuis closed this Aug 15, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.