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: deflake test-http-destroyed-socket-write2 #36120

Merged
merged 1 commit into from Nov 18, 2020
Merged

test: deflake test-http-destroyed-socket-write2 #36120

merged 1 commit into from Nov 18, 2020

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 14, 2020 •
edited

Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

Fixes: #36081
Fixes: #4066

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 14, 2020
@lpinca
Copy link
Member Author

lpinca commented Nov 14, 2020 

./tools/test.py -J --repeat=500 test/parallel/test-http-destroyed-socket-write2.js

was failing before with timeouts on macOS, now it exits with 0.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@nodejs-github-bot
Copy link
Collaborator

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

rickyes
rickyes reviewed Nov 15, 2020
View reviewed changes
test/parallel/test-http-destroyed-socket-write2.js
});
}
server.once(kResponseDestroyed, common.mustCall(function() {
req.write('hello');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for .mustCall here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise there is no guarantee that the listener of the 'request' event added on line 33 is called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway we can use common.mustCall() there (line 33) if it is easier to grok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misunderstood. Do you mean adding a callback to this req.write()? If so, I think it is not needed. It's out of the scope of the test.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
test: deflake test-http-destroyed-socket-write2
e682814 
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

PR-URL: nodejs#36120
Fixes: nodejs#36081
Fixes: nodejs#4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@lpinca lpinca merged commit e682814 into nodejs:master Nov 18, 2020
@lpinca
Copy link
Member Author

lpinca commented Nov 18, 2020

Landed in e682814.

@lpinca lpinca deleted the fix/36081 branch November 18, 2020 17:52
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@lpinca @codebytere
test: deflake test-http-destroyed-socket-write2
4746649 
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

PR-URL: #36120
Fixes: #36081
Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@lpinca @BethGriggs
test: deflake test-http-destroyed-socket-write2
fa14657 
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

PR-URL: #36120
Fixes: #36081
Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@lpinca @BethGriggs
test: deflake test-http-destroyed-socket-write2
ed1a114 
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

PR-URL: #36120
Fixes: #36081
Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@lpinca @BethGriggs
test: deflake test-http-destroyed-socket-write2
3e77536 
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

PR-URL: #36120
Fixes: #36081
Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@rickyes rickyes rickyes approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test-http-destroyed-socket-write2 timeout Investigate flaky test-http-destroyed-socket-write2
4 participants
@lpinca @nodejs-github-bot @Trott @rickyes