doc: add warning for socket.connect reuse

[ronag]

socket.connect is subtly broken due to timing and _undestroy issues. Discourage usage and ask users to instead create a new socket instance.

Note, this is a problem when calling connect a second time.

I believe since it's a bit tricky to resolve, it might be appropriate to deprecate this use case since there is a viable alternative in simply creating a new socket.

Refs: #25969

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

Might have to deprecate new net.Socket as well for this to make sense, or make it so new net.Socket always connects automatically.

[sam-github]

I appreciate the subtle problems it introduces in terms of state, but thorough review of ecosystem use-cases would be required, this is a long-standing feature.

cc: @bnoordhuis who might have perspective on uses this is put to

[mcollina]

lgtm

[mscdex]

-1 Creating a new socket is not always a solution, especially if there are event handlers to carry over, user-added properties to carry over, etc. I think it would be better to come up with a solution to fix whichever edge cases instead of deprecating socket.connect().

[mscdex]

In addition, I think there are options that can only be passed to the net.Socket constructor, so end users would no longer be able to supply those?

[jasnell]

I favor this direction but there are a ton of details to figure out.

[ronag]

Creating a new socket is not always a solution, especially if there are event handlers to carry over, user-added properties to carry over, etc.

But it is possible to workaround this. Just create your own socket wrapper.

I think it would be better to come up with a solution to fix whichever edge cases instead of deprecating socket.connect().

This has been broken for quite a while and no one has fixed it. I think it's better to deprecate than leaving it broken.

In addition, I think there are options that can only be passed to the net.Socket constructor, so end users would no longer be able to supply those?

What options are those? We should fix so that net.connect forwards those.

[ronag]

but there are a ton of details to figure out.

Anything specific in mind? Other than the constructor situation?

[lpinca]

Addressing a regression (#25969) with a method deprecation does not seem the proper way to fix it to me.

What's wrong with socket.connect() beside the issue described in #25969 which did not exist before 53fb7af1b2?

[ronag]

Addressing a regression (#25969) with a method deprecation does not seem the proper way to fix it to me.

This has been an issue for a long time. Last time we discussed it we basically came to the conclusion that this use case should be very unusual and its not worth fixing. It just became relevant again given that issue.

What's wrong with socket.connect()

When re-using the socket, i.e. calling connect() a second time the state is kind of messed up, i.e. it is possible to re-use the socket before the state is in a state where it actually can be reused. The behavior at this point is kind of undefined.

[bnoordhuis]

this is a long-standing feature

That's an understatement. :-)

socket.connect() was added in 2009, when net.Socket was still called net.Stream (0618f02) and it's been around ever since.

I agree with Luigi that deprecation because of bugs isn't a fix. The original connect() method worked fine, it's all the extra layers that have been piled on over the years that cause trouble. undestroy in particular just feels like a Bad Idea.

I appreciate it's non-trivial to untangle but deprecation seems like the wrong direction.

[ronag]

I appreciate it's non-trivial to untangle but deprecation seems like the wrong direction.

Sorry to be a bit negative here. But I don't really see a way to fix this entirely. There simply is too much state and edge cases going on, at least for me.

The way I see it we have the following options:

1. Deprecate it.
2. Leave it broken, possibly with a "here be dragons" comment in docs.
3. Somebody puts some effort into making it less bad / good enough. I am sceptical whether we can fully fix it.

Given that 3. could take a while, I think in the short-medium term we need to do either 1 or 2. I'm fine with either.

I agree with Luigi that deprecation because of bugs isn't a fix. The original connect() method worked fine, it's all the extra layers that have been piled on over the years that cause trouble. undestroy in particular just feels like a Bad Idea.

As far as I can see, it has been subtly broken since at least 1.x (haven't checked further back). It's still "undestroying" even if it's inline. Depending on when exactly you call Socket.connect() the behavior is pretty much undefined. Though, maybe it hasn't been a problem in practice? I don't know...

[bnoordhuis]

I don't recall any bug reports so perhaps it works well enough in practice. If you think it's inherently unfixable (I don't necessarily disagree) then documenting the caveats is a good idea.

[ronag]

then documenting the caveats is a good idea.

I don't know what the exact caveats are. As far as I'm aware it's undefined behavior. So basically the documentation would be "here be dragons".

The problem here is that there can be pending events from either IO or process.nextTick that are side effects from before the connect call but would occurr after. Exactly which events and callbacks and what the consequences are is difficult to figure out but most likely it would corrupt state. Furthermore it totally depends on the timing of when connect is called which makes it unpredictable. Basically the state becomes undefined.

I'm not even sure what the user would expect in terms of behavior when connect() is called... should 'close' or any other event be emitted or not? Should it fail if there is an existing connection or close it? What about pending writes/reads/callbacks etc...

I guess what I'm trying to say here is, we know it's broken, but not exactly how, we don't know the exact semantics of how we would like it to behave and it's difficult to fix so that it "reasonably" works.

[ronag]

deprecation because of bugs isn't a fix

I think the question here is; what should we do with an API we know (at least I'm personally convinced) is broken and we are unlikely to fix (at least I'm personally sceptical).

Maybe I'm a bit ahead of things and there is someone that thinks they can reasonably (whatever that means) fix it within an acceptable (whatever that means) timeframe? It's been broken for a very long time so I guess there is no hurry. On the other hand I'm personally not comfortable leaving it broken without telling our users.

[lpinca]

We can document that reusing a socket after it has been closed is not safe instead of deprecating socket.connect(). Why can't I use

constsocket=newSocket(options);socket.connect();

instead of

constsocket=net.connect(options);

which does the same thing and still uses socket.connect() under the hood?

[ronag]

We can document that reusing a socket after it has been closed is not safe instead

I'm fine with this.

EDIT: Though I would like to be able to sometime in the future throw when called for re-use.

[ronag]

@lpinca: PTAL and see if you agree with the current wording.

[ronag]

@sam-github@jasnell@mscdex@bnoordhuis PTAL

[sam-github]

If a user waits for close to be emitted on a socket, isn't it known that there are no pending events, and connect is safe? In other words, is it not that calling connect on a currently connected socket that leads to chaos, not calling connect on a socket that used to be connected but isn't any longer?

[ronag]

If a user waits for close to be emitted on a socket, isn't it known that there are no pending events, and connect is safe? In other words, is it not that calling connect on a currently connected socket that leads to chaos, not calling connect on a socket that used to be connected but isn't any longer?

In theory yes. In practice, not necessarily.

For example we have e.g. a bug in node < 15 where 'finish' is emitted after 'close' in certain cases for net.Socket. Though, I guess it is fixed in v15. I haven't dug into whether there are other edge cases for this.

EDIT: #33204 (comment)

[ronag]

I would be fine with changing the wording to "socket.connect() is only safe after'close'". Even if that is not the case right now... that is something which should be achievable.

[lpinca]

For example we have e.g. a bug in node < 15 where 'finish' is emitted after 'close' in certain cases for net.Socket

Can you show an example of this?

[ronag]

Can you show an example of this?

Sorry, I got confused by #33137 which is actually another issue.

[sam-github]

I personally would prefer the more limited and informative description in #33204 (comment), so people understand under what circumstances they are really asking for trouble, and what should work.

Perhaps not the place to discuss, but if calling .connect() on a currently connected and active socket is particularly dangerous, I think its worth considering whether that very specific use could be deprecated, even runtime deprecated. That would require that it is possible to know that the socket is not in a quiescent and disconnected state. If that is possible, perhaps issuing a run-time warning that the use is not safe would be reasonable. I suspect that people are not regularly doing the unsafe connect, we'd be flooded with bug reports. <-- just a thought, an aside from this documentation here.

[ronag]

@sam-github updated wording PTAL

[mcollina]

lgtm

[ronag]

Landed in e50ae7a