-
Notifications
You must be signed in to change notification settings - Fork 30k
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
net: add new connection attempts events #51045
net: add new connection attempts events #51045
Conversation
Review requested:
|
e372bf7
to
7546326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I would not make these events public. If Happy Eyeballs will be moved to a lower level (libuv) in the future, it will be complex/useless to keep supporting them. |
@lpinca Are you just talking about removing them from the documentation? |
Yes, or using private symbols. We can make them public later, if needed. |
doc/api/net.md
Outdated
Emitted when a connection attempt failed. This may be emitted multiple times | ||
if the family autoselection algorithm is enabled in [`socket.connect(options)`][]. | ||
|
||
### Event: `'connectionAttemptFailed'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be connectionAttemptTimeout
or whatever the third event was called. This appears to be an accidental duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. Copy and paste lazyness. Fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One doc nit that needs fixing, otherwise LGTM
I presume these are added now because they are useful/needed now. Whether happy eyeballs is ever moved into libuv is still hypothetical AFAIK. I would rather not gate on this based on that hypothetical. We can deprecate these events in the future if necessary. |
I have no objections, but if the events are only needed internally for testing/debugging why making them public? If people start using/abusing them, it will be hard to deprecate/remove them. |
@lpinca The events are, in my opinion, not just needed internally. At the moment we don't really have any events other than |
aa8f6f8
to
de721be
Compare
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: TODO
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
added: REPLACEME | ||
--> | ||
|
||
* `ip` {number} The IP which the socket is attempting to connect to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShogunPanda Looks like it should be * address {string}
. Not number
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. This has been fixed in #51490.
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
This looks like it's been baking in v21.6.0 for a few weeks now - is it going to be back-ported to v20 at all? (I noticed the lts-watch tag so I assume "probably"?) |
It should: I think this will happen in couple of weeks. |
it didn't get merged yet with the latest v20.11.1, didn't it? 🫤 |
No it was a security release, there will be a release in a couple of weeks |
@marco-ippolito any news on the next 20.x release? There is no release proposal for it yet, so at least another two weeks? |
@marco-ippolito could you please provide an update. We're now approaching 4 weeks since the latest update requests, which remained unanswered. |
Looks like this has released in 20.12.0 |
This PR adds three new events in the
net.createConnection
flow:connectionAttempt
: Emitted when a new connection attempt is established. In case of Happy Eyeballs, this might emitted multiple times.connectionAttemptFailed
: Emitted when a connection attempt failed. In case of Happy Eyeballs, this might emitted multiple times.connectionAttemptTimeout
: Emitted when a connection attempt timed out. In case of Happy Eyeballs, this will not be emitted for the last attempt. This is not emitted at all if Happy Eyeballs is not used.Additionally, a previous bug has been fixed where a new connection attempt could have been started after a previous one failed and after the connection was destroyed by the user. This led to a failed assertion.
This bug was reported several times but all were marked duplicates of the issue below.
Note that without the new events (especially
connectionAttemptTimeout
) I think is impossible to correctly test the bugfix.Fixes: #48763