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

test: ensure finish is emitted before destroy #33137

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 28, 2020

Adds a test to ensure that 'finish' is emitted
before the socket is destroyed by allow half-open
enforcer.

Refs: 3c07b17#commitcomment-38810268

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

Adds a test to ensure that 'finish' is emitted
before the socket is destroyed by allow half-open
enforcer.

Refs: nodejs@3c07b17#commitcomment-38810268
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 28, 2020
@ronag
Copy link
Member Author

ronag commented Apr 28, 2020

review with ignore whitespace

@ronag ronag added the net Issues and PRs related to the net subsystem. label Apr 28, 2020
@ronag
Copy link
Member Author

ronag commented Apr 28, 2020

This is a problem on v14. So we should backport this test together with a fix. Unfortunately the semver-major fix on master (#31806) cannot be backported.

@ronag ronag requested a review from lpinca April 28, 2020 21:55
socket.on('end', common.mustCall(() => {
assert(!socket.destroyed);
}));
socket.end('asd');
Copy link
Member Author

@ronag ronag Apr 29, 2020

Choose a reason for hiding this comment

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

calling end() here is the main difference with the previous test


{
const server = net.createServer(common.mustCall((socket) => {
socket.end(Buffer.alloc(1024));
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed now that the end() is also called on the client? Shouldn't this socket be resumed to consume the data sent by the client?

Copy link
Member Author

@ronag ronag Apr 29, 2020

Choose a reason for hiding this comment

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

Not sure, it does trigger the bug on v14 in the current form though...

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added the review wanted PRs that need reviews. label May 4, 2020
@BridgeAR
Copy link
Member

@nodejs/streams @nodejs/http @nodejs/http2 this needs some reviews. It's open for quite a while by now.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Ping @nodejs/streams

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but definitely would like @mcollina's opinion

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

ronag added a commit that referenced this pull request Jun 24, 2020
Adds a test to ensure that 'finish' is emitted
before the socket is destroyed by allow half-open
enforcer.

Refs: 3c07b17#commitcomment-38810268

PR-URL: #33137
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ronag
Copy link
Member Author

ronag commented Jun 24, 2020

Landed in 2ccf15b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. review wanted PRs that need reviews. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants