Backport 48969 to v18.x staging v2 #49183
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
PR-URL: nodejs#46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#48189 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@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>
mhdawson
force-pushed
the
backport-48969-to-v18.x-staging-v2
branch
from
0a79941
to
afa5fd7
Compare
|
Based on discussion with @ruyadorno squashed last commit into the PR it belonged with and removed the blank |
github-actions
bot
removed
the
request-ci
|
This is likely a real issue. @ShogunPanda before I go looking in the haystack do you have any context that would point me in the direction of which PR might have affected it. @ |
|
Based on in the action it looks like the internet tests are only run when the internet tests are passed. I'm not sure that makes sense as if this PR introduces a failure, the tests were only run because of an unrelated change to a diffferent test in /test/internet. It also makes looking at history hard as you need a PR that changed the internet tests to compare to. |
|
Running in an unbuntu 22 container just the one test passes: root@cf00edeb225c:/io.js# tools/test.py test/internet/test-dns-ipv6 All tests passed. I'm guessing since IPV6 is not configured for the container? |
|
Looking at the address that is being complained about There seems to be an extra ':' between the last two parts. Wondering if that is the issue. |
|
The file from which the address comes has the extra colon - Line 46 in 6f9d6f2 |
|
Nevermind I guess that the double colons is what is expected and it is a properly formed address based on |
|
In my non-containered machine where IPV6 is at least partically configured (but not fully configured which is why it did not use it for initial tests), the test fails even after I remove all of the commits in this PR. |
|
I removed all of the commits in the staging branch that are not in the release 18.x branch and still got the same failure on my local machine. |
|
PR to see if test failure re-occurs in CI to rule out my environment being the cause of the failures I'm seeing - #49191 |
|
The test-dns-ipv6 test fails in the same way on the shipped v18.x branch with the only change being adding a blank line to a different test in internet to trigger the tests running - see #49191. Therefore, I believe that failure is not related to the commits in this PR. |
|
This was the failure in the initial CI run on PPC, don't believe it is related to the changes in this PR Test Result (1 failure / +1) Going to resume build. |
|
From some initial bisection, the test-dns-ipv16 test starts failing as of 18.17.0 |
|
I believe my investigation confirms that the failure in test-dns-ipv16 is not related to the content in this PR and should not be a blocker. I'll open an issue for the pre-existing test-dns-ipv16 but I think @ruyadorno this PR should be ready based on the rest of the CI passing. |
@mhdawson is there a need to open an issue to track the |
And switch from `google.com` to `nodejs.org`. PR-URL: #47029 Backport-PR-URL: #49183 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>
|
Landed in 0beb5ab...3eeca52 |
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: #49183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
|
@ruyadorno should have mentioned I opened #49203 to track the |
This is the set of commits needed to backport 48969 to v18.x
The list of PR's addressed in the commits include:
net: rework autoSelectFamily implementation #46587 (merge conflicts - merged)
net: fix family autoselection SSL connection handling #48189 (merge conflicts - merged)
test: move
test-tls-autoselectfamily-servernametotest/internet#47029 (applied cleanly)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.
I added a blank
Backport-PR-URL:to the commits that required a merge48969 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.