Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add warning for socket.connect reuse #33204

Closed
wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented May 1, 2020

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 ronag requested a review from mcollina May 1, 2020 21:41
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 1, 2020
`socket.connect` is subtly broken due to timing and
_undestroy issues. Discourage usage and ask users
to instead create a new socket instance.

Refs: nodejs#33203
@ronag ronag force-pushed the deprecate-socket-connect branch from d448671 to 76bf3dd Compare May 1, 2020 21:41
@ronag
Copy link
Member Author

ronag commented May 1, 2020

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
Copy link
Contributor

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

@ronag ronag requested a review from bnoordhuis May 1, 2020 22:12
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label May 1, 2020
@mscdex
Copy link
Contributor

mscdex commented May 1, 2020

-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
Copy link
Contributor

mscdex commented May 1, 2020

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
Copy link
Member

jasnell commented May 2, 2020

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

@ronag
Copy link
Member Author

ronag commented May 2, 2020

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
Copy link
Member Author

ronag commented May 2, 2020

but there are a ton of details to figure out.

Anything specific in mind? Other than the constructor situation?

@lpinca
Copy link
Member

lpinca commented May 2, 2020

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
Copy link
Member Author

ronag commented May 2, 2020

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
Copy link
Member

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
Copy link
Member Author

ronag commented May 4, 2020

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
Copy link
Member

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
Copy link
Member Author

ronag commented May 5, 2020

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
Copy link
Member Author

ronag commented May 5, 2020

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
Copy link
Member

lpinca commented May 5, 2020

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

const socket = new Socket(options);
socket.connect();

instead of

const socket = net.connect(options);

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

@ronag
Copy link
Member Author

ronag commented May 5, 2020

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
Copy link
Member Author

ronag commented May 5, 2020

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

@ronag ronag force-pushed the deprecate-socket-connect branch 5 times, most recently from 2071fb9 to b4c24ef Compare May 5, 2020 11:07
@ronag
Copy link
Member Author

ronag commented May 5, 2020

doc/api/net.md Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor

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
Copy link
Member Author

ronag commented May 5, 2020

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
Copy link
Member Author

ronag commented May 5, 2020

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
Copy link
Member

lpinca commented May 5, 2020

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
Copy link
Member Author

ronag commented May 5, 2020

Can you show an example of this?

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

@sam-github
Copy link
Contributor

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 ronag force-pushed the deprecate-socket-connect branch from 0382a34 to 3d26329 Compare May 6, 2020 20:42
@ronag
Copy link
Member Author

ronag commented May 6, 2020

@sam-github updated wording PTAL

@ronag ronag force-pushed the deprecate-socket-connect branch from 3d26329 to 74430ce Compare May 6, 2020 20:43
@ronag ronag removed the notable-change PRs with changes that should be highlighted in changelogs. label May 8, 2020
@ronag ronag requested review from lpinca and jasnell May 8, 2020 11:00
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 8, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag
Copy link
Member Author

ronag commented May 8, 2020

Landed in e50ae7a

@ronag ronag closed this May 8, 2020
ronag added a commit that referenced this pull request May 8, 2020
PR-URL: #33204
Refs: #25969
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request May 11, 2020
PR-URL: #33204
Refs: #25969
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request May 18, 2020
@ronag ronag mentioned this pull request May 27, 2020
5 tasks
codebytere pushed a commit that referenced this pull request Jun 7, 2020
PR-URL: #33204
Refs: #25969
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants