[v18.x] Backport 48969 to v18.x staging #49016
Closed
Conversation
This file contains 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#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>
|
Review requested:
|
nodejs-github-bot
added
meta
|
I think this PR has something wrong with 5k+ changes file |
RafaelGSS
changed the title
RafaelGSS
added
the
v18.x
|
@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. |
|
@RafaelGSS many thanks, I was wondering what I'd done wrong based on @mcollina comments but see I had used the wrong base. |
RafaelGSS
force-pushed
the
v18.x-staging
branch
from
a05f72f
to
e7d2e8e
Compare
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. |
|
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
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
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>
|
Landed in 863bdb7...ab7ca07 |
|
Thanks @mhdawson ❤️ |
|
@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
force-pushed
the
v18.x-staging
branch
from
ab7ca07
to
d3637cd
Compare
|
@mhdawson let's open a new PR to add the remainder commits instead 😊 |
|
replaced by #49183 |
This was referenced Aug 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.