Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upnet: use `_final` instead of `on('finish')` #18608
Conversation
nodejs-github-bot
added
the
net
label
Feb 7, 2018
lpinca
approved these changes
Feb 7, 2018
| return self._handle.shutdown(req); | ||
| } | ||
|
|
||
| // the user has called .end(), and all the bytes have been | ||
| // sent out to the other side. | ||
| function onSocketFinish() { | ||
| Socket.prototype._final = function(cb) { | ||
| // If still connecting - defer handling 'finish' until 'connect' will happen |
This comment has been minimized.
This comment has been minimized.
lpinca
Feb 7, 2018
Member
Nit: I'd remove handling 'finish' as we are no longer handling the 'finish' event.
This comment has been minimized.
This comment has been minimized.
addaleax
Feb 7, 2018
Author
Member
@lpinca done! And thanks for the reviews, it’s really helpful to talk this through with you!
addaleax
referenced this pull request
Feb 7, 2018
Closed
net: inline and simplify onSocketEnd #18607
jasnell
approved these changes
Feb 9, 2018
This comment has been minimized.
This comment has been minimized.
|
@addaleax please always trigger a CI after opening a PR :-) |
This comment has been minimized.
This comment has been minimized.
I usually wait until the first review or so, since the PR likely needs to be updated after it anyway. |
This comment has been minimized.
This comment has been minimized.
addaleax
added
the
author ready
label
Feb 16, 2018
BridgeAR
requested a review
from
mcollina
Feb 17, 2018
| @@ -55,6 +55,8 @@ function undestroy() { | |||
| this._writableState.destroyed = false; | |||
| this._writableState.ended = false; | |||
| this._writableState.ending = false; | |||
| this._writableState.finalCalled = false; | |||
| this._writableState.prefinished = false; | |||
This comment has been minimized.
This comment has been minimized.
mcollina
Feb 17, 2018
Member
can you add a unit test for those? Maybe also place them in a separate commit, if we want to backport them separately.
This comment has been minimized.
This comment has been minimized.
addaleax
Feb 19, 2018
Author
Member
@mcollina Do you know where those tests are? In any case, these lines are tested in the sense that tests do fail without them...
This comment has been minimized.
This comment has been minimized.
mcollina
Feb 20, 2018
Member
We need test that can be run as part of readable-stream.
Here are the current tests:
test/parallel/test-stream-duplex-destroy.js
test/parallel/test-stream-readable-destroy.js
test/parallel/test-stream-transform-destroy.js
test/parallel/test-stream-writable-destroy.js
BridgeAR
removed
the
author ready
label
Feb 18, 2018
addaleax
added some commits
Feb 7, 2018
addaleax
force-pushed the
addaleax:net-final
branch
from
461d558
to
356ac44
Feb 20, 2018
This comment has been minimized.
This comment has been minimized.
|
@mcollina Thanks for the pointer, done! |
addaleax
added
the
author ready
label
Feb 20, 2018
This comment has been minimized.
This comment has been minimized.
|
Landed in 906bbef |
addaleax
closed this
Feb 21, 2018
addaleax
deleted the
addaleax:net-final
branch
Feb 21, 2018
addaleax
removed
the
author ready
label
Feb 21, 2018
This comment has been minimized.
This comment has been minimized.
|
@addaleax it seems this landed without metadata. |
This comment has been minimized.
This comment has been minimized.
|
@lpinca I’ve force-pushed that mistake away, thanks for pointing it out. |
addaleax commentedFeb 7, 2018
Shutting down the connection is what
_finalis there for.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
net