net: wrong docs for socket 'connect' event registration

[Legiew]

I'am playing around with a few issues in the net module.

As said in the docs the parameter 'connectionListener' (callback) in the functions net.createConnection, net.connect and socket.connect will be added as an listener for the 'connect' event.

If you have a look in the socket.connect function, which is called from every of these functions, the 'connect' event is fired only once. (net.js line 864: self.once('connect', cb);) It is not saved or added as a permanent listener.

I think this should be corrected in the docs. If you create a socket with the net.createConnection function and reuse this socket and connect again with the socket.connect function you have to register again for the 'connect' event. If you rely on the docs you only will get one event fired.

I can make a PR for this, but first wanted to get some comments about this cause I am not 100% sure if I got this right.

[sam-github]

Sounds like a bug, not a doc problem, can you post a runnable gist to repro?

[Legiew]

Hey,

sry that it took me so long, but I had no time to work on this.

Here is a gist to reproduce: https://gist.github.com/Legiew/46a54f686e7fd2421d39

You get the following:
server bound
Connect Event 1
client connected
client disconnected
Closed
reconnect
Connect Event 2
client connected
client disconnected
Closed

Following the current docs I would expect this instead:
server bound
Connect Event 1
client connected
client disconnected
Closed
reconnect
Connect Event 1
Connect Event 2
client connected
client disconnected
Closed

[silverwind]

I also think this is more of a code bug, but if we change that behaviour there could possibly be a lot of broken modules relying on the current behavior. It looks like the .once listener also is applied to socket, and possible other places too (tls and the various createServer). I'd keep the change doc only for the sake of compatibilty.

[bnoordhuis]

There's a good reason for using .once(). If .on() was used instead and the event listener holds a reference to (for example) a 100 MB buffer, that buffer will stay around for the lifetime of the socket.

Due to the way V8 allocates closure contexts for inner functions, your listener may be holding onto objects it doesn't even reference directly. That's why, as a rule of thumb, listeners should stay attached for the shortest period possible.

I move we document the current behavior.

PS: 'inner functions' includes top-level functions in modules and scripts. Those are wrapped in an outer function by the module loader.

[bnoordhuis]

Documented in fbaef40.