stream: fix flaky stream destroy, reachable from HTTP/2 teardown - #65079
stream: fix flaky stream destroy, reachable from HTTP/2 teardown#65079pimterry wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Signed-off-by: Tim Perry <pimterry@gmail.com>
|
cc @nodejs/http2 |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #65079 +/- ##
==========================================
+ Coverage 90.29% 90.32% +0.03%
==========================================
Files 759 759
Lines 247598 248458 +860
Branches 46680 46869 +189
==========================================
+ Hits 223566 224420 +854
+ Misses 15503 15448 -55
- Partials 8529 8590 +61
... and 87 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Stress testing inconclusive - the tests passed on both main and this branch, even with multiple runs. They're definitely failing in PRs though. I can reliably reproduce the reported failure locally (that first failure above, it hits for 2-3% of runs) and validate that it's fixed with this change. The trace is where is crashes on my machine with the matching error, and it's clearly a broken flow, so I think this is the right fix regardless. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
I concur with the analysis
Sorry, something went wrong.
Sorry, something went wrong.
|
Even with this PR's change I can still fail test-stream-pipeline-http2.js with a timeout 31/1000 times. Without it about 56/1000 times. This diff that claude spit out to deflake test-stream-pipeline-http2.js makes that 0/1000 with or without this PR, but I don't know enough about this subsystem to say whether that retains the point of the test or not. diff --git a/test/parallel/test-stream-pipeline-http2.js b/test/parallel/test-stream-pipeline-http2.js
--- a/test/parallel/test-stream-pipeline-http2.js
+++ b/test/parallel/test-stream-pipeline-http2.js
@@ -27,10 +27,11 @@
- let cnt = 10;
+ let received = 0;
req.on('data', (data) => {
- cnt--;
- if (cnt === 0) rs.destroy();
+ received += data.length;
+ // HTTP/2 data event boundaries are non-deterministic.
+ if (received >= 32 * 1024) rs.destroy();
});
|
Sorry, something went wrong.
Yes, sorry if the description isn't clear - both failing tests are related to that PR, but this PR only fixes the first of the two. I.e. test-worker-terminate-http2-respond-with-file. I haven't opened a fix for the second yet, because I think it's actually exposed a real bug that's a bit complicated, and mac-only. Looks like that fix would resolve the test, but from what I can tell so far there's a real underlying deadlock that's reproducible independently, so I want to get a proper fix for that instead. I'll update when I have more info there. |
Sorry, something went wrong.
|
@pimterry can you land this with the fix/workaround i posted to the remaining flake knowing you'll revisit the underlying deadlock problem and possibly change it again? |
Sorry, something went wrong.
This does not solve the remaining underlying deadlock issue, but does bound the test behaviour in a way that seems to avoid failures in practice. Deadlock fix to come separately later. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com>
|
Good plan @panva, now done 👍. I'll open the other fix separately. I've got it working now but the deadlock comes from code we added to resolve past CVEs, so needs some thought and it'd be nice not to rush it. |
Sorry, something went wrong.
This comment was marked as outdated.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65079 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This does not solve the remaining underlying deadlock issue, but does bound the test behaviour in a way that seems to avoid failures in practice. Deadlock fix to come separately later. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65079 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65079 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This does not solve the remaining underlying deadlock issue, but does bound the test behaviour in a way that seems to avoid failures in practice. Deadlock fix to come separately later. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65079 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This does not solve the remaining underlying deadlock issue, but does bound the test behaviour in a way that seems to avoid failures in practice. Deadlock fix to come separately later. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65079 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:
Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.
This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.
Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:
Worker::Run → FreeEnvironment → Environment::RunCleanup → BaseObjectList::Cleanup → fs::FileHandle::~FileHandle → StreamResource::~StreamResource → StreamPipe::ReadableListener::OnStreamDestroy → StreamPipe::ReadableListener::OnStreamReadI.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.
This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).
I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.
Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().