Skip to content

test: fix flaky test-http2-reset-flood#34318

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.

Sign up for GitHub

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

Jump to bottom
Closed
Trott wants to merge 2 commits into from
Closed

test: fix flaky test-http2-reset-flood #34318

Trott wants to merge 2 commits into from

Conversation

@Trott
Copy link
Member

@TrottTrott commented Jul 12, 2020

Set allowHalfOpen: true in the client.

Fixes: #29802
Refs: #31806

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott
test: fix flaky test-http2-reset-flood
9e95a46
Set `allowHalfOpen: true` in the client. Fixes: nodejs#29802 Refs: nodejs#31806
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jul 12, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 12, 2020•
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32292/ ✅

@Trott
Copy link
MemberAuthor

Trott commented Jul 12, 2020•
edited
Loading

Stress test this PR (should be green): https://ci.nodejs.org/job/node-stress-single-test/111/ ✅

Stress test master branch (should be very red): https://ci.nodejs.org/job/node-stress-single-test/112/ ❌

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 12, 2020•
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32295/ ✅

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 12, 2020
@lpinca
Copy link
Member

lpinca commented Jul 12, 2020•
edited
Loading

Why allowHalfOpen was not needed before #31806? I mean #31806 was not meant to change net.Socket behavior no?

@Trott
Copy link
MemberAuthor

Trott commented Jul 14, 2020

Why allowHalfOpen was not needed before #31806? I mean #31806 was not meant to change net.Socket behavior no?

A similar comment was left by @addaleax on the original PR: #31806 (comment)

/cc @ronag

@ronag
Copy link
Member

ronag commented Jul 14, 2020•
edited
Loading

I mean #31806 was not meant to change net.Socket behavior no?

It's a semver major... the timing of things might have changed. I believe onReadableStreamEnd was partly broken before and could cause 'finish' to be emitted after 'close' or something along those lines. Not sure anymore.

The full conversation is here https://github.com/nodejs/node/pull/31806/files#r386140400
Maybe related? #33137

@addaleax
Copy link
Member

addaleax commented Jul 14, 2020

Landed in 4195c31

addaleax pushed a commit that referenced this pull request Jul 14, 2020
@Trott@addaleax
test: fix flaky test-http2-reset-flood
4195c31
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]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
@Trott@MylesBorins
test: fix flaky test-http2-reset-flood
c72585b
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]>
@MylesBorinsMylesBorins mentioned this pull request Jul 14, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
@Trott@MylesBorins
test: fix flaky test-http2-reset-flood
28ce378
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]>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@Trott@cjihrig
test: fix flaky test-http2-reset-flood
9fa53b3
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]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott@addaleax
test: fix flaky test-http2-reset-flood
de83997
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]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott@addaleax
test: fix flaky test-http2-reset-flood
9741510
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]>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
Merged
@TrottTrott deleted the fix-29802 branch April 14, 2022 11:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@ronagronagronag approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky test-http2-reset-flood

5 participants

@Trott@nodejs-github-bot@lpinca@ronag@addaleax