stream: always invoke end callback by ronag · Pull Request #28687 · nodejs/node

ronag · 2019-07-14T20:11:37Z

Refs: #28667

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ronag · 2019-07-14T20:11:44Z

@mcollina ping

lib/_stream_writable.js

addaleax · 2019-07-14T20:27:30Z

I’m being a bit careful here and adding the semver-major label … if somebody disagrees, we can remove it

nodejs-github-bot · 2019-07-14T20:28:08Z

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

ronag · 2019-07-14T21:24:31Z

Um, that CI fail is a bit strange? It seems to have run against an intermediate/WIP commit? How do I restart it?

lib/_stream_writable.js

ronag · 2019-07-14T21:49:15Z

also applied same fix to OutgoingMessage

nodejs-github-bot · 2019-07-15T05:04:01Z

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

Trott · 2019-07-15T05:36:00Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1907/

lib/_stream_writable.js

ronag · 2019-07-15T11:48:49Z

@lpinca @mcollina I tried a different approach that I hope you maybe would agree more with?

I'll cleanup the PR if you prefer this solution.

lib/_stream_writable.js

mcollina

I'm sorry for the bike shedding on the error name, but I think ERR_STREAM_FINISHED might confuse users because we have a finished(stream) utility. So, how about ERR_STREAM_ALREADY_FINISHED?

doc/api/errors.md

lpinca · 2019-07-15T17:34:51Z

Tests please :)
Thank you.

trivikr

LGTM

test/parallel/test-http-outgoing-writableFinished.js

ronag · 2019-07-17T21:04:33Z

@mcollina approve?

mcollina

LGTM

ronag · 2019-08-19T10:16:39Z

@Trott: This looks ready

nodejs-github-bot · 2019-08-19T14:55:07Z

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

Trott · 2019-08-19T14:55:10Z

CITGM (queued): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1966/

nodejs-github-bot · 2019-08-19T17:35:20Z

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

Trott · 2019-08-19T18:05:07Z

CITGM (queued): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1966/

A few new failures but I think they're due to other changes on master. Here's a CITGM against master for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1967/ (queued)

ronag · 2019-08-19T19:00:25Z

@Trott: please let me know if I need to investigate failures

Invoke callback with ERR_STREAM_ALREADY_FINISHED error if `end()` is called on a finished stream. PR-URL: #28687 Refs: #28667 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2019-08-20T15:51:24Z

Landed in db706da

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Jul 14, 2019

ronag force-pushed the stream-end-always-invoke-callback branch from e56801d to d10b09d Compare July 14, 2019 20:12

addaleax reviewed Jul 14, 2019

View reviewed changes