-
Notifications
You must be signed in to change notification settings - Fork 30k
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
flaky test-http2-reset-flood #29802
Comments
This test is somewhat regularly flaky on 12.x and it appears master as well. There appears to be an issue tracking flakes for 6+ months. Seems reasonable to mark this to keep us from having red CI. Refs: nodejs#29802
This test is somewhat regularly flaky on 12.x and it appears master as well. There appears to be an issue tracking flakes for 6+ months. Seems reasonable to mark this to keep us from having red CI. Refs: #29802 PR-URL: #32595 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
This test is somewhat regularly flaky on 12.x and it appears master as well. There appears to be an issue tracking flakes for 6+ months. Seems reasonable to mark this to keep us from having red CI. Refs: #29802 PR-URL: #32595 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
We are seeing this as flaky on 12.x on a bunch of other linux distros as well https://ci.nodejs.org/job/node-test-commit/37126/ Not sure what we should do about this |
@addaleax also seeing this fail on windows now too (all on 12.x). Do you think this is a real failure? Should we mark this flaky across all release lines? Expand scope of this issue? |
Here is an example of it failing across almost every platform |
@MylesBorins The v12.x failures seem very different from the failure on master. I could reproduce the v12.x failures locally on Linux. This patch fixes it for me and shouldn’t affect the test’s functionaliy: diff --git a/test/parallel/test-http2-reset-flood.js b/test/parallel/test-http2-reset-flood.js
index 7ee534cf4faa..d721384ab374 100644
--- a/test/parallel/test-http2-reset-flood.js
+++ b/test/parallel/test-http2-reset-flood.js
@@ -69,9 +69,10 @@ const worker = new Worker(__filename).on('message', common.mustCall((port) => {
h2header.writeIntBE(streamId, 5, 4); // Stream ID
streamId += 2;
// 0x88 = :status: 200
- conn.write(Buffer.concat([h2header, Buffer.from([0x88])]));
+ if (conn.writable)
+ conn.write(Buffer.concat([h2header, Buffer.from([0x88])]));
}
- if (!gotError)
+ if (conn.writable && !gotError)
setImmediate(writeRequests);
}
I’m not sure why it’s not needed on master but it’s probably because @ronag put a lot of work into streams finally making sense. Either way, feel free to take that patch and open a PR with it 🙂 |
Refs: #29802 (comment) PR-URL: #32607 Reviewed-By: Anna Henningsen <[email protected]>
This test is somewhat regularly flaky on 12.x and it appears master as well. There appears to be an issue tracking flakes for 6+ months. Seems reasonable to mark this to keep us from having red CI. Refs: #29802 PR-URL: #32595 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Failed in master here: https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora-last-latest-x64/34123/ |
This test is somewhat regularly flaky on 12.x and it appears master as well. There appears to be an issue tracking flakes for 6+ months. Seems reasonable to mark this to keep us from having red CI. Refs: #29802 PR-URL: #32595 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
still flaky on various platforms, linuxone in this case https://ci.nodejs.org/job/node-test-pull-request/30678/ |
failed in https://ci.nodejs.org/job/node-test-commit-linux-containered/19466/, linux-containered, x86 |
Refs: #29802 Refs: #32595 PR-URL: #32825 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Refs: #29802 Refs: #32595 PR-URL: #32825 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Refs: nodejs#29802 Refs: nodejs#32595 PR-URL: nodejs#32825 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Refs: #29802 Refs: #32595 PR-URL: #32825 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Refs: #29802 Refs: #32595 PR-URL: #32825 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Timed out at https://ci.nodejs.org/job/node-test-commit-linux/36018/nodes=debian9-64/console on test-digitalocean-debian9-x64-1:
|
Move boolean check to start of writeRequest() to avoid race condition. Fixes: nodejs#29802
I changed the test to track calls and to timeout after two seconds (because it normally takes about 100 milliseconds on the debian9 machine in CI) and it is showing things like 167450 calls to the test's Here's the modified test I ran: https://github.com/nodejs/node/blob/09713d420bfd65ab550840eb55e10e4996bbcba2/test/parallel/test-http2-reset-flood.js And here's a sample failure in the stress test on CI:
The key part is that first line that shows that I had previously inserted a bunch of |
The timeouts we're seeing now are different than the issue that was first reported (which appears to have been fixed). I did a bisect using CI today (since it's easy to make this happen in the stress tests) and came up with efefdd6 as the first commit where the problem was introduced. I'll do some more tests to confirm, but if someone knowledgable wants to take a look there, that would be great. |
Setting |
Set `allowHalfOpen: true` in the client. Fixes: nodejs#29802 Refs: nodejs#31806
Set `allowHalfOpen: true` in the client. Fixes: #29802 Refs: #31806 PR-URL: #34318 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
Set `allowHalfOpen: true` in the client. Fixes: #29802 Refs: #31806 PR-URL: #34318 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
Set `allowHalfOpen: true` in the client. Fixes: #29802 Refs: #31806 PR-URL: #34318 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
Set `allowHalfOpen: true` in the client. Fixes: #29802 Refs: #31806 PR-URL: #34318 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
Set `allowHalfOpen: true` in the client. Fixes: #29802 Refs: #31806 PR-URL: #34318 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
From #29727, which seems wholly unrelated, since it changes the link flags on linux, and this is a freebsd failure.
@nodejs/platform-freebsd
The text was updated successfully, but these errors were encountered: