http: don't emit end after aborted #27984

ronag · 2019-05-30T18:25:59Z

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

lib/_http_client.js

ronag · 2019-05-30T20:53:14Z

I realized we don't need to call destroy since all it does is destroy the socket and since we are inside the socket's close listener it is already destroying...

ronag · 2019-05-30T21:14:47Z

Another option is to set readable to false on aborted.

ronag · 2019-05-30T21:23:09Z

Actually, I think it might be best if as much data as possible is read before aborted is emitted?

ronag · 2019-05-30T21:25:29Z

I kind of want something like this. But I'm not sure about the prependListener hack... is there a better way?

    req.emit('close');
    if (!res.complete) {
      // Socket closed before we emitted 'end' below.
      // Read as much as possible before emitting.
      res.prependListener('end', function() {
        res.off('end');
        this.emit('aborted');
        this.emit('close');
      });
      res.push(null);
    } else if (res.readable) {
      res.on('end', function() {
        this.emit('close');
      });
      res.push(null);
    } else {
      res.emit('close');
    }

nodejs-github-bot · 2019-06-02T08:10:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23547/
CI: https://ci.nodejs.org/job/node-test-pull-request/23839/
CI: https://ci.nodejs.org/job/node-test-pull-request/23861/

Trott · 2019-06-02T12:50:25Z

Test failures in CI--http tests now timing out.

ronag · 2019-06-02T13:04:56Z

@Trott trying to fix but I'm no longer able to build master?

/Users/ronagy/GitHub/public/node/out/Release/obj.target/v8_snapshot/geni/embedded.cc:5:10: fatal error: 'src/snapshot/macros.h' file not found

Trott · 2019-06-02T13:33:42Z

@Trott trying to fix but I'm no longer able to build master?

/Users/ronagy/GitHub/public/node/out/Release/obj.target/v8_snapshot/geni/embedded.cc:5:10: fatal error: 'src/snapshot/macros.h' file not found

@nodejs/build

lib/_http_client.js

lpinca · 2019-06-04T20:16:12Z

It seems a spurious test.js file was commited.

lpinca

SMGT with the spurious test.js removed and commit title fixed.
I'm not sure if it should be marked as semver-major though.

ronag · 2019-06-04T20:35:13Z

what's wrong with the commit title?

lpinca · 2019-06-05T05:51:33Z

"Fix" should be "fix".

Trott · 2019-09-30T20:11:44Z

I ran locally against pumpify and it failed there too. I'm going to remove the "Request Changes" and bisect to find out when it broke....

broken CITGM stuff was already broken, will troubleshoot elsewhere....

Trott · 2019-09-30T20:14:35Z

(csv-parser isn't broken for me locally though.... once I'm done bisecting for pumpify, I'll apply this change to confirm it doesn't break csv-parser for me locally, out of extreme caution... )

Trott · 2019-09-30T20:32:12Z

CITGM on master shows pumpify and csv-parser issues. sigh

Trott · 2019-09-30T21:15:34Z

@Trott: I can try and help out. Is there a chat or something so we can communicate?

I'm often in nodeslackers and in Freenode IRC. Twitter DMs are pretty good for me too.

ronag · 2019-10-04T20:01:38Z

@Trott: How are we doing here?

Trott · 2019-10-05T20:00:13Z

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

Recent CITGM run on master for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2031/

Trott · 2019-10-11T16:12:08Z

CITGM results look worse than they are because AIX ran out of memory. If regular CI is green, I think this is good to land.

Trott · 2019-10-11T16:13:53Z

(I'd prefer some details in the commit message body but I wouldn't consider that blocking.)

nodejs-github-bot · 2019-10-14T01:55:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26008/ ✔️

IncomingMessage will no longer emit end after aborted. PR-URL: #27984 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2019-10-14T02:07:07Z

Landed in 5f80df8

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 May 30, 2019

ronag changed the title ~~Fix end after aborted~~ http: Fix end after aborted May 30, 2019

ronag force-pushed the fix-end-after-aborted branch 2 times, most recently from 817aca3 to 0eafe44 Compare May 30, 2019 18:28

mscdex reviewed May 30, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

ronag force-pushed the fix-end-after-aborted branch from 0eafe44 to 5294bb7 Compare May 30, 2019 20:00

mscdex reviewed May 30, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

ronag force-pushed the fix-end-after-aborted branch 5 times, most recently from b5d6265 to eeda729 Compare May 30, 2019 20:52

ronag force-pushed the fix-end-after-aborted branch from eeda729 to c5a575e Compare May 30, 2019 21:22

ronag force-pushed the fix-end-after-aborted branch 2 times, most recently from 5fdc7cd to e333056 Compare May 30, 2019 21:24

ronag force-pushed the fix-end-after-aborted branch from e333056 to 2d0a03f Compare May 30, 2019 23:01

lpinca reviewed Jun 4, 2019

View reviewed changes

lib/_http_client.js Show resolved Hide resolved

lpinca approved these changes Jun 4, 2019

View reviewed changes

ronag force-pushed the fix-end-after-aborted branch from 2d0a03f to aa558d9 Compare June 4, 2019 20:35

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

ronag requested a review from Trott October 4, 2019 06:29

This comment has been minimized.

Sign in to view

Trott closed this Oct 14, 2019

BethGriggs mentioned this pull request Oct 15, 2019

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

Merged

jonchurch mentioned this pull request Mar 6, 2020

Some tests fail on Node.js v13 ladjs/superagent#1548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: don't emit end after aborted #27984

http: don't emit end after aborted #27984

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019

ronag commented May 30, 2019 •

edited

Loading

nodejs-github-bot commented Jun 2, 2019 •

edited

Loading

Trott commented Jun 2, 2019

ronag commented Jun 2, 2019

Trott commented Jun 2, 2019

lpinca commented Jun 4, 2019

lpinca left a comment •

edited

Loading

ronag commented Jun 4, 2019

lpinca commented Jun 5, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

ronag commented Oct 4, 2019

Trott commented Oct 5, 2019

This comment has been minimized.

Trott commented Oct 11, 2019

Trott commented Oct 11, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 14, 2019 •

edited by Trott

Loading

Trott commented Oct 14, 2019

http: don't emit end after aborted #27984

http: don't emit end after aborted #27984

Conversation

ronag commented May 30, 2019 • edited Loading

ronag commented May 30, 2019 • edited Loading

ronag commented May 30, 2019 • edited Loading

ronag commented May 30, 2019

ronag commented May 30, 2019 • edited Loading

nodejs-github-bot commented Jun 2, 2019 • edited Loading

Trott commented Jun 2, 2019

ronag commented Jun 2, 2019

Trott commented Jun 2, 2019

lpinca commented Jun 4, 2019

lpinca left a comment • edited Loading

Choose a reason for hiding this comment

ronag commented Jun 4, 2019

lpinca commented Jun 5, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

Trott commented Sep 30, 2019

ronag commented Oct 4, 2019

Trott commented Oct 5, 2019

This comment has been minimized.

Trott commented Oct 11, 2019

Trott commented Oct 11, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 14, 2019 • edited by Trott Loading

Trott commented Oct 14, 2019

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019 •

edited

Loading

ronag commented May 30, 2019 •

edited

Loading

nodejs-github-bot commented Jun 2, 2019 •

edited

Loading

lpinca left a comment •

edited

Loading

nodejs-github-bot commented Oct 14, 2019 •

edited by Trott

Loading