Skip to content
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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

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

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

@Trott
Copy link
Member Author

Trott commented Jul 12, 2020 •
edited

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

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

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

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

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