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: fix flaky test-net-write-after-close #14361

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 19, 2017

Replace 250ms timer with event-based logic to make test robust.

Fixes: #13597

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

test net

@Trott Trott added flaky-test Issues and PRs related to the tests with unstable failures on the CI. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Jul 19, 2017
@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

}));

server.listen(0, function() {
const client = net.connect(this.address().port, function() {
client.on('end', () => {
serverSocket.write('test', common.mustCall());
});
Copy link
Member

Choose a reason for hiding this comment

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

Maybe wrapping this with common.mustCall()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

}));

server.listen(0, function() {
const client = net.connect(this.address().port, function() {
client.on('end', () => {
Copy link
Member

Choose a reason for hiding this comment

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

'end' signals the end of the readable side of the stream, I think it should be 'finish' (see https://nodejs.org/api/stream.html#stream_events_finish_and_end).

Copy link
Member Author

@Trott Trott Jul 19, 2017

Choose a reason for hiding this comment

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

Should documentation for the finish event be added to net.Socket documentation the way end is? Or would that just be more confusing than helpful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test fails if I replace end with finish because serverSocket is undefined...

/Users/trott/io.js/test/parallel/test-net-write-after-close.js:43
      serverSocket.write('test', common.mustCall());
                  ^

TypeError: Cannot read property 'write' of undefined
    at Socket.client.on (/Users/trott/io.js/test/parallel/test-net-write-after-close.js:43:19)
    at emitNone (events.js:110:20)
    at Socket.emit (events.js:207:7)
    at finishMaybe (_stream_writable.js:587:14)
    at endWritable (_stream_writable.js:595:3)
    at Socket.Writable.end (_stream_writable.js:546:5)
    at Socket.end (net.js:491:31)
    at Socket.<anonymous> (/Users/trott/io.js/test/parallel/test-net-write-after-close.js:45:12)
    at Object.onceWrapper (events.js:314:30)
    at emitNone (events.js:105:13)

Copy link
Member

Choose a reason for hiding this comment

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

Actually now that I read this better, 'end' is correct.

'finish' is part of the streams API, but it will signal when the current side is closed. 'end'  will be emitted when the other side as stopped sending, so when we call serverSocker.write() that will lead to an error.

const server = net.createServer(common.mustCall(function(socket) {
serverSocket = socket;

socket.resume();

socket.on('error', common.mustCall(function(error) {
console.error('got error, closing server', error);
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

it seems this is the expected behavior. Can we change the text or place a comment so it's clear to the reader?

Copy link
Member Author

Choose a reason for hiding this comment

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

got error -> received error as expected

Copy link
Member Author

Choose a reason for hiding this comment

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

Also: I'd be OK with removing the console.error() entirely too. Either way.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

% comments

}));

server.listen(0, function() {
const client = net.connect(this.address().port, function() {
client.on('end', () => {
serverSocket.write('test', common.mustCall());
});
client.end();
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment on top of this? Something like:

// cliend.end() will close both the readable and writable side
// of the duplex, because allowHalfOpen is defaulted to false.
// Then 'end' will be emitted when it receives a FIN packet from
// the other side.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Done.

@Trott Trott force-pushed the fix-13597 branch 2 times, most recently from 5696f73 to e1a978a Compare July 19, 2017 13:47
Replace 250ms timer with event-based logic to make test robust.

Fixes: nodejs#13597
@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

Stress tests:

@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

Re-running CI with changes:

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

@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

Stress tests results are as expected. 🎉

@Trott
Copy link
Member Author

Trott commented Jul 21, 2017

Landed in 43bd47c

Trott added a commit to Trott/io.js that referenced this pull request Jul 21, 2017
Replace 250ms timer with event-based logic to make test robust.

PR-URL: nodejs#14361
Fixes: nodejs#13597
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott closed this Jul 21, 2017
addaleax pushed a commit that referenced this pull request Jul 22, 2017
Replace 250ms timer with event-based logic to make test robust.

PR-URL: #14361
Fixes: #13597
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
Replace 250ms timer with event-based logic to make test robust.

PR-URL: #14361
Fixes: #13597
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@Trott Trott deleted the fix-13597 branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: investigate flakiness of test-net-write-after-close
8 participants