test: fix flaky test-net-write-after-close#14361
Conversation
There was a problem hiding this comment.
Maybe wrapping this with common.mustCall()?
There was a problem hiding this comment.
'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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
it seems this is the expected behavior. Can we change the text or place a comment so it's clear to the reader?
There was a problem hiding this comment.
got error -> received error as expected
There was a problem hiding this comment.
Also: I'd be OK with removing the console.error() entirely too. Either way.
There was a problem hiding this comment.
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.5696f73 to
e1a978a
Compare
Replace 250ms timer with event-based logic to make test robust. Fixes: nodejs#13597
|
Stress tests:
|
|
Re-running CI with changes: |
|
Stress tests results are as expected. 🎉 |
|
Landed in 43bd47c |
Replace 250ms timer with event-based logic to make test robust. PR-URL: nodejs#14361 Fixes: nodejs#13597 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Replace 250ms timer with event-based logic to make test robust. PR-URL: #14361 Fixes: #13597 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Replace 250ms timer with event-based logic to make test robust. PR-URL: #14361 Fixes: #13597 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
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 |
Replace 250ms timer with event-based logic to make test robust.
Fixes: #13597
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test net