http: don't emit 'data' after 'error' #28711

ronag · 2019-07-15T23:06:38Z

Don't emit pending 'data' after 'error'.

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

jasnell · 2019-07-17T14:59:26Z

Defensively marking as semver-major due to the change in event timing. I'd prefer to view this as a bug fix tho. @mcollina what do you think?

mcollina

why this change? I kind of agree that we should do it.. but I don’t understand what problems is this causing to users, not this
is explained. Would you mind expanding on this?

ronag · 2019-07-17T16:08:19Z

The implicit assumption for streams is (as we discussed elsewhere) that we only emit close after error.

Here is a potential case where this can be a problem:

req
 .on('error', err => {
   fs.close(file)
 })
 .on('data', buf => {
   // uh oh... we might already be closed...
   fs.write(file, buf)
 })

apapirovski

I actually don’t agree with this order. Arguably knowing that there was an error and marking the socket is more important than emitting whatever data was leftover.

I’m open to the alternative but it’s unclear what the benefit is — given there’s been no feedback on this over the years.

ronag · 2019-07-17T16:18:31Z

I actually don’t agree with this order. Arguably knowing that there was an error and marking the socket is more important than emitting whatever data was leftover.

The alternative is that we just dump any pending data. Makes sense to me. Should I change it?

mcollina · 2019-07-17T16:48:42Z

I would prefer if we dumped any pending data. It actually make more sense.

ronag · 2019-07-17T18:12:15Z

Updated

ronag · 2019-07-17T21:05:10Z

@apapirovski approve?

ronag · 2019-08-18T07:56:53Z

@benjamingr @Trott this has stalled? ping node/http?

Trott · 2019-08-18T08:57:54Z

@benjamingr @Trott this has stalled? ping node/http?

@ronag Should the commit message be updated? This is more about not reading data on the error condition than ordering of events?

@jasnell Is this still a breaking change in your opinion?

@apapirovski Are you still opposed on the grounds that this has never been reported as a significant real-world problem? Or with the change as it is now, do you think this is something we should do? Similar question for @mcollina

ronag · 2019-08-18T09:36:08Z

@Trott: Rebased and fixed message.

I think @mcollina and @apapirovski's objections were resolved with the current fix, i.e. don't emit data after error.

mcollina

LGTM

Trott · 2019-08-18T22:36:30Z

/ping @nodejs/http for review/comments

Trott

Seems OK to me if CITGM doesn't find anything

Trott · 2019-08-18T22:38:34Z

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

ronag · 2019-08-23T10:30:51Z

@Trott: this looks ready

mcollina · 2019-08-26T09:07:38Z

@apapirovski @lpinca what do you think about this change?

nodejs-github-bot · 2019-09-18T02:11:56Z

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

Trott · 2019-09-18T02:12:14Z

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

Trott · 2019-09-18T02:12:52Z

@apapirovski Would love to get your 👍 , 👎 , or ¯\(ツ)/¯ on this

Trott · 2019-09-18T04:20:05Z

CITGM results seem OK.

nodejs-github-bot · 2019-09-18T04:20:17Z

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

nodejs-github-bot · 2019-09-18T04:47:21Z

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

Trott · 2019-09-18T12:52:55Z

I actually don’t agree with this order. Arguably knowing that there was an error and marking the socket is more important than emitting whatever data was leftover.

I’m open to the alternative but it’s unclear what the benefit is — given there’s been no feedback on this over the years.

Given that this PR no longer emits data after error, I think @apapirovski's objection has been addressed. Multiple pings have not brought him back into this conversation, so I'm going to clear the objection based on https://github.com/nodejs/node/blob/a0c6cf8eb1bbe4b964b8267341e894a8fafeab51/COLLABORATOR_GUIDE.md#consensus-seeking.

Let's leave this open another 48 hours or so to see if @apapirovski or anyone else has anything more to say. (I'll ping @apapirovski through non-GitHub channels just in case the problem here is being overwhelmed by GitHub notifications or whatever.) But as of now, this has sufficient approvals, a good CTIGM run, and a good CI run.

data is no longer emitted after error

PR-URL: #28711 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Trott · 2019-09-23T01:09:00Z

Landed in e573c39

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

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 15, 2019

ronag mentioned this pull request Jul 15, 2019

emit after 'error' #28710

Closed

ronag force-pushed the http-data-before-error branch from a964829 to e25e1ba Compare July 16, 2019 07:22

jasnell approved these changes Jul 17, 2019

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 17, 2019

mcollina reviewed Jul 17, 2019

View reviewed changes

apapirovski previously requested changes Jul 17, 2019

View reviewed changes

ronag force-pushed the http-data-before-error branch from e25e1ba to 115cd68 Compare July 17, 2019 18:11

ronag force-pushed the http-data-before-error branch 3 times, most recently from 7ab4da0 to 6d1a9c3 Compare July 17, 2019 18:13

This was referenced Aug 6, 2019

stream: cleanup #29000

Closed

http: ClientRequest streamlike #29002

Closed

ronag force-pushed the http-data-before-error branch from 6d1a9c3 to d7c31ae Compare August 9, 2019 16:05

http: don't emit 'data' after 'error'

be5940b

ronag changed the title ~~http: emit pending 'data' before 'error'.~~ http: don't emit 'data' after 'error'. Aug 18, 2019

ronag force-pushed the http-data-before-error branch from d7c31ae to be5940b Compare August 18, 2019 09:35

ronag changed the title ~~http: don't emit 'data' after 'error'.~~ http: don't emit 'data' after 'error' Aug 18, 2019

mcollina approved these changes Aug 18, 2019

View reviewed changes

Trott approved these changes Aug 18, 2019

View reviewed changes

trivikr requested review from apapirovski and jasnell August 18, 2019 23:27

jasnell approved these changes Aug 19, 2019

View reviewed changes

trivikr approved these changes Aug 19, 2019

View reviewed changes

lpinca approved these changes Aug 26, 2019

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2019

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

Trott closed this Sep 23, 2019

BethGriggs mentioned this pull request Sep 25, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: don't emit 'data' after 'error' #28711

http: don't emit 'data' after 'error' #28711

ronag commented Jul 15, 2019 •

edited

Loading

jasnell commented Jul 17, 2019

mcollina left a comment

ronag commented Jul 17, 2019 •

edited

Loading

apapirovski left a comment

ronag commented Jul 17, 2019

mcollina commented Jul 17, 2019

ronag commented Jul 17, 2019

ronag commented Jul 17, 2019

ronag commented Aug 18, 2019 •

edited

Loading

Trott commented Aug 18, 2019 •

edited

Loading

ronag commented Aug 18, 2019 •

edited

Loading

mcollina left a comment

Trott commented Aug 18, 2019

Trott left a comment

Trott commented Aug 18, 2019

ronag commented Aug 23, 2019

mcollina commented Aug 26, 2019

nodejs-github-bot commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 18, 2019

nodejs-github-bot commented Sep 18, 2019

nodejs-github-bot commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 23, 2019

http: don't emit 'data' after 'error' #28711

http: don't emit 'data' after 'error' #28711

Conversation

ronag commented Jul 15, 2019 • edited Loading

Checklist

jasnell commented Jul 17, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Jul 17, 2019 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

ronag commented Jul 17, 2019

mcollina commented Jul 17, 2019

ronag commented Jul 17, 2019

ronag commented Jul 17, 2019

ronag commented Aug 18, 2019 • edited Loading

Trott commented Aug 18, 2019 • edited Loading

ronag commented Aug 18, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Aug 18, 2019

Trott left a comment

Choose a reason for hiding this comment

Trott commented Aug 18, 2019

ronag commented Aug 23, 2019

mcollina commented Aug 26, 2019

nodejs-github-bot commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 18, 2019

nodejs-github-bot commented Sep 18, 2019

nodejs-github-bot commented Sep 18, 2019

Trott commented Sep 18, 2019

Trott commented Sep 23, 2019

ronag commented Jul 15, 2019 •

edited

Loading

ronag commented Jul 17, 2019 •

edited

Loading

ronag commented Aug 18, 2019 •

edited

Loading

Trott commented Aug 18, 2019 •

edited

Loading

ronag commented Aug 18, 2019 •

edited

Loading