Conversation
|
nice but we have to be carefull because any other pull will likely revert it |
calvinmetcalf
left a comment
calvinmetcalf
left a comment
There was a problem hiding this comment.
older node is breaking
lib/_stream_writable.js
Outdated
| process.nextTick(cb, er); | ||
| // this can emit finish, and it will always happen | ||
| // after error | ||
| process.nextTick(finishMaybe, stream, state); |
There was a problem hiding this comment.
this needs to be processNextTick
lib/_stream_writable.js
Outdated
| if (sync) { | ||
| // defer the callback if we are being called synchronously | ||
| // to avoid piling up things on the stack | ||
| process.nextTick(cb, er); |
There was a problem hiding this comment.
needs to be processNextTick
|
lgtm, other than the other comments, with the caveats that i'm 100% up to date with the new mechanics. Are there any guarantees now about close firing before after finish etc? |
I have no idea. I think the two things are loosely correlated in duplexes. Given the rate of unexpected breakage, I think we have poor coverage of the state machine and the ecosystem. |
3 participants
So, here is the forward-port of that PR. It will likely be part of Node 8.1.3, which will likely be released next week. I'm not sure if it's worth porting it forward here.
nodejs/node#13850
cc @phated