Skip to content

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

Closed
Trott wants to merge 2 commits intonodejs:masterfrom
Trott:fix-29802
Closed

test: fix flaky test-http2-reset-flood#34318
Trott wants to merge 2 commits intonodejs:masterfrom
Trott:fix-29802

Conversation

@Trott
Copy link
Member

@Trott Trott 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-bot nodejs-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
Member Author

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/ ✅

@Trott Trott 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
Member Author

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 addaleax closed this Jul 14, 2020
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@MylesBorins MylesBorins 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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@Trott Trott deleted the fix-29802 branch April 14, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@ronag ronag ronag approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues 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