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

[v18.x] Backport 48969 to v18.x staging #49016

Closed

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Aug 4, 2023

This is the set of commits needed to backport 48969 to v18.x

It is based off of #48275 which @juanarbol created to backport #48258. We can either land this after that lands or add the commits in this PR to that existing PR. Will leave that up to the releasers to decide which is best.

The list of PR's addressed in the commits include:

46587 required a merge so I added the Backport-PR-URL: to that commit but without the actual URL. 48969 also required a tweak to the test even though it applied cleanly. That is in the final commit in this PR.

@ShogunPanda I tried to include 48464 a few ways, but the obvious merge to me resulted in many failures. It would be good to have somebody with more context do backport for that one.

48969 has not landed in 20.x yet but I'm hoping that will be the case before the next 18.x release.

targos and others added 9 commits July 23, 2023 17:23
The test's assumptions about RSS are no longer valid, at least with
Fedora 38.

Closes: nodejs#48490
PR-URL: nodejs#48811
Fixes: nodejs#48490
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#45777
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.

PR-URL: nodejs#48258
Backport-PR-URL: nodejs#48275
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Previously persistence was completed disabled if you removed this
header, which is not correct for modern HTTP, where the header is
optional and all connections should persist by default regardless.

PR-URL: nodejs#46331
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
PR-URL: nodejs#46587
Backport-PR-URL:
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
And switch from `google.com` to `nodejs.org`.

PR-URL: nodejs#47029
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Fixs two issues in `TLSWrap`, one of them is reported in
nodejs#30896.

1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.

2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.

However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
  tls.connect({socket: tlssock})
or
  tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.

The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.

PR-URL: nodejs#48969
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
- APLN option is not supported on 18.x so remove

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Aug 4, 2023
@mcollina
Copy link
Member

mcollina commented Aug 4, 2023

I think this PR has something wrong with 5k+ changes file

@RafaelGSS RafaelGSS changed the base branch from main to v18.x-staging August 4, 2023 15:08
@RafaelGSS RafaelGSS changed the title Backport 48969 to v18.x staging [v18.x] Backport 48969 to v18.x staging Aug 4, 2023
@RafaelGSS RafaelGSS added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Aug 4, 2023
@RafaelGSS
Copy link
Member

@mhdawson I took the liberty of changing the base and updating the PR according to the correct flags for backport. I hope you do not mind.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 4, 2023

@RafaelGSS many thanks, I was wondering what I'd done wrong based on @mcollina comments but see I had used the wrong base.

@nodejs-github-bot
Copy link
Collaborator

lib/net.js Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

Pushed commit to fix the linter complaint. If that results in green I'll see if I can get it merged into 568975f where it probably should have been done.

@ruyadorno
Copy link
Member

don't worry about the proper fix @mhdawson I already have it properly merged in my wip!

ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #45777
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #47860
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.

PR-URL: #48258
Backport-PR-URL: #49016
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
Previously persistence was completed disabled if you removed this
header, which is not correct for modern HTTP, where the header is
optional and all connections should persist by default regardless.

PR-URL: #46331
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #46587
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
And switch from `google.com` to `nodejs.org`.

PR-URL: #47029
Backport-PR-URL: #49016
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
Fixs two issues in `TLSWrap`, one of them is reported in
#30896.

1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.

2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.

However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
  tls.connect({socket: tlssock})
or
  tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.

The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.

PR-URL: #48969
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@ruyadorno
Copy link
Member

Landed in 863bdb7...ab7ca07

@ruyadorno
Copy link
Member

Thanks @mhdawson ❤️

@mhdawson mhdawson reopened this Aug 14, 2023
@mhdawson
Copy link
Member Author

@ruyadorno found that I'd included a SemVer major PR in the list 46331. I only added that to make later PRs need less merges so will fix the PR to omit that one.

@ruyadorno
Copy link
Member

@mhdawson let's open a new PR to add the remainder commits instead 😊

@ruyadorno ruyadorno closed this Aug 15, 2023
@mhdawson
Copy link
Member Author

replaced by #49183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.