lib: use Writable#_final for 'finish' tracking
#27
Conversation
|
I didn't run the indutny@Fedors-MacBook-Pro-2 dicer % git co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 419.234ms
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 432.782ms
indutny@Fedors-MacBook-Pro-2 dicer % git co fix/pipeline-incompat
Switched to branch 'fix/pipeline-incompat'
Your branch is up to date with 'sig/fix/pipeline-incompat'.
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 418.913ms
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 423.359ms |
|
FWIW, I've tested it on our codebase (Signal-Desktop) and the |
indutny-signal
force-pushed
the
fix/pipeline-incompat
branch
from
0ab918d
to
632253f
Compare
|
Welp, sorry for CI failures. Force pushed. |
|
Oh, it is actually failing on older node.js versions... Weird. |
|
Ah, |
Instead overriding `emit` method and using `this._realFinish` to detect
unexpected end of data - implement `_final` method and track the logic
there.
The only behavior change from the use of `_final` is that we would no
longer emit `finish` when the input data is terminated early. Instead we
would emit `error` as before and stop the processing. Note that this is
the behavior provided by `stream.Writable`, and it is thus conformant
with the specification.
Additionally, replace uses of private `_events` property with either
straight `emit()` with return value check, or `listenerCount()` in the
situations where there might be an overhead from the constructed event
arguments.
Replace `emit('error', error)` with `destroy(error)` calls as well, as
otherwise the behavior is undefined.
Fix: mscdex#26
indutny-signal
force-pushed
the
fix/pipeline-incompat
branch
from
632253f
to
abd843e
Compare
|
Oof, this was tricky. Node 10 and 12 have different event behavior from 14 and 16. In particular, I get both Additionally, I had to delay the Weird stuff! |
|
Anyway, it is all fixed now, although I'm much less happy with the PR than I was at the start. (Hacks 😭) |
|
Yay, all green! 😁 |
|
Totally no rush with this. Just let me know if you'll ever need any additional information or more detailed description of the changes to help you review this. Thanks! |
|
(Friendly weekly reminder so that this PR doesn't fall through the cracks of your inbox) |
|
(Friendly reminder after one month 😉) |
|
(This PR is still open 😁 ) |
|
I ran into this issue today, using |
Instead overriding
emitmethod and usingthis._realFinishto detectunexpected end of data - implement
_finalmethod and track the logicthere.
The only behavior change from the use of
_finalis that we would nolonger emit
finishwhen the input data is terminated early. Instead wewould emit
erroras before and stop the processing. Note that this isthe behavior provided by
stream.Writable, and it is thus conformantwith the specification.
Additionally, replace uses of private
_eventsproperty with eitherstraight
emit()with return value check, orlistenerCount()in thesituations where there might be an overhead from the constructed event
arguments.
Fix: #26
cc @mscdex (this is @indutny under disguise)