[v18.x] Backport 48969 to v18.x staging#49016
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#45777 Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#47860 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
And switch from `google.com` to `nodejs.org`. PR-URL: nodejs#47029 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
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 <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
- APLN option is not supported on 18.x so remove Signed-off-by: Michael Dawson <[email protected]>
Collaborator
nodejs-github-bot commented Aug 4, 2023
Review requested:
|
nodejs-github-bot added meta 
Member
mcollina commented Aug 4, 2023
I think this PR has something wrong with 5k+ changes file |
RafaelGSS changed the title RafaelGSS added the v18.x 
Member
RafaelGSS commented Aug 4, 2023
@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. |
MemberAuthor
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. |
Collaborator
nodejs-github-bot commented Aug 11, 2023
mhdawson commented Aug 14, 2023
Uh oh!
There was an error while loading. Please reload this page.
MemberAuthor
mhdawson commented Aug 14, 2023
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. |
Member
ruyadorno commented Aug 14, 2023
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 <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #47860 Backport-PR-URL: #49016 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #46587 Backport-PR-URL: #49016 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
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 <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
Member
ruyadorno commented Aug 14, 2023
Landed in 863bdb7...ab7ca07 |
Member
ruyadorno commented Aug 14, 2023
Thanks @mhdawson ❤️ |
MemberAuthor
mhdawson commented Aug 14, 2023
@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. |
Member
ruyadorno commented Aug 15, 2023
@mhdawson let's open a new PR to add the remainder commits instead 😊 |
MemberAuthor
mhdawson commented Aug 15, 2023
replaced by #49183 |
This was referenced Aug 17, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
http: keep HTTP/1.1 conns alive even if the Connection header is removed #46331 (applied cleanly) - needed for 47029 to apply cleanly
net: rework autoSelectFamily implementation #46587 (merge conflicts - merged)
test: move
test-tls-autoselectfamily-servernametotest/internet#47029 (applied cleanly)net: fix family autoselection SSL connection handling #48189 (applies clean)
tls: fix bugs of double TLS #48969 applied clean but needs additional commit to fix test as ALPN not part of 18.x. Added as additional commit.
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.