http: fix stalled pipeline bug by indutny · Pull Request #3342 · nodejs/node

indutny · 2015-10-13T06:19:53Z

This is a two-part fix:

Fix pending data notification in OutgoingMessage to notify server
about flushed data too
unconsume socket from http parser on pause, to prevent parser from
reporting PAUSED error. Previously, no data events were emitted
after socket.pause() call. Now with all things happening in C++,
socket.pause() has no effect on internal socket while it is
consumed. Switching to old mode is enough for now, we should revisit
it somewhere later.

Fix: #3332

R=@bnoordhuis @jasnell @trevnorris

cc @nodejs/collaborators

indutny · 2015-10-13T06:21:03Z

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

indutny · 2015-10-13T06:21:13Z

The test may be failing on windows, because it is a bit CPU intensive.

indutny · 2015-10-13T06:21:27Z

Going to sleep now, will reply to all comments tomorrow.

thefourtheye · 2015-10-13T10:48:06Z

lib/_http_outgoing.js

Wouldn't a better check be if (typeof this._onPendingData === 'function')?

I'll move the checks together and replaced it with typeof, thanks

indutny · 2015-10-13T15:07:50Z

Pushed one fix, any other comments?

indutny · 2015-10-13T15:41:26Z

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

jasnell · 2015-10-13T15:54:19Z

LGTM assuming CI is green

* Includes fixes for two regressions - Assertion error in WeakCallback - see [#3329](#3329) - Undefined timeout regression - see [#3331](#3331) * Document an additional known issue with pipelined requests - See: #3332 and #3342

indutny · 2015-10-13T16:18:40Z

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

* Includes fixes for two regressions - Assertion error in WeakCallback - see [#3329](#3329) - Undefined timeout regression - see [#3331](#3331) * Document an additional known issue with pipelined requests - See: #3332 and #3342

trevnorris · 2015-10-13T17:57:35Z

Fix pending data notification in OutgoingMessage to notify server about flushed data too

The "too" at the end makes me feel like there's more to the sentence.

trevnorris · 2015-10-13T17:58:45Z

lib/_http_outgoing.js

Is checking for typeof function part of a fix, or different cleanup?

It is a cleanup.

trevnorris · 2015-10-13T18:13:00Z

LGTM

indutny · 2015-10-13T18:17:05Z

Thanks @trevnorris . I'm working on fixing the test on Windows. For some reason it fails with EALREADY

jasnell · 2015-10-13T19:51:21Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/499/

jasnell · 2015-10-13T20:28:14Z

CI Issues:

Linting: https://ci.nodejs.org/job/node-linter/961/nodes=linter/console
Likely unrelated failure in freebsd: https://ci.nodejs.org/job/node-test-commit-other/921/nodes=freebsd101-64/console
Likely unrelated failure in win10: https://ci.nodejs.org/job/node-test-commit-windows/938/nodes=win10/console

trevnorris · 2015-10-13T20:34:30Z

lib/_http_outgoing.js

This part is in OutgoingMessage#_writeRaw() but not in OutgoingMessage#_finish(). Is this already addressed in the git commit message? Don't see any direct reference to why this is necessary for the fix.

You meant _flush?

Ah oop. Yeah _flush.

I think it is used there too.

trevnorris · 2015-10-13T21:03:22Z

Overall LGTM but have a few questions.

indutny · 2015-10-13T23:56:42Z

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

indutny · 2015-10-13T23:58:44Z

lib/_http_outgoing.js

cc @trevnorris

Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) #2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) #3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) #3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) #3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) #3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) #2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) #3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) #2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) #3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) #3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) #3407. * tls: - Added ALPN Support (Shigeki Ohtsu) #2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) #2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) #1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) #3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) #3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) #2595. PR-URL: #3466

indutny added http Issues or PRs related to the http subsystem. land-on-v4.x labels Oct 13, 2015

indutny mentioned this pull request Oct 13, 2015

Issue with Node 4.x when receiving pipelined reqeusts #3332

Closed

thefourtheye reviewed Oct 13, 2015
View reviewed changes

indutny mentioned this pull request Oct 13, 2015

2015-10-13, Version 4.2.1 'Argon' (LTS) Release #3337

Closed

trevnorris reviewed Oct 13, 2015
View reviewed changes

indutny reviewed Oct 13, 2015
View reviewed changes

lib/_http_outgoing.js

Copy link

Member Author

indutny Oct 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @trevnorris

glortho mentioned this pull request Oct 30, 2015

WS continuation frames & node > 4.0 http-party/node-http-proxy#881

Open

jasnell removed the land-on-v4.x label Nov 3, 2015

snyk-bot mentioned this pull request Mar 16, 2020

[Snyk] Upgrade rollup from 0.62.0 to 1.31.1 luke7oaks/node-1#2

Open

snyk-bot mentioned this pull request Sep 25, 2022

[Snyk] Fix for 22 vulnerabilities 137717unity/node#70

Open

wedataintelligence mentioned this pull request Sep 26, 2022

[Snyk] Fix for 22 vulnerabilities wedataintelligence/node#44

Open

137717unity mentioned this pull request Sep 26, 2022

[Snyk] Fix for 22 vulnerabilities 137717unity/node#73

Open

snyk-bot mentioned this pull request Sep 27, 2022

[Snyk] Fix for 21 vulnerabilities Rednickle/node-1#56

Open

wedataintelligence mentioned this pull request Sep 29, 2022

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#50

Open

This was referenced Sep 29, 2022

[Snyk] Fix for 24 vulnerabilities 137717unity/node#75

Open

[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#60

Open

This was referenced Oct 5, 2022

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#51

Open

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#52

Open

snyk-bot mentioned this pull request Oct 6, 2022

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#53

Open

137717unity mentioned this pull request Oct 7, 2022

[Snyk] Fix for 24 vulnerabilities 137717unity/node#76

Open

wedataintelligence mentioned this pull request Oct 7, 2022

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#54

Open

This was referenced Oct 8, 2022

[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#55

Open

[Snyk] Fix for 24 vulnerabilities 137717unity/node#77

Open

137717unity mentioned this pull request Oct 9, 2022

[Snyk] Fix for 24 vulnerabilities 137717unity/node#78

Open

snyk-bot mentioned this pull request Oct 9, 2022

[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#63

Open

kissofire mentioned this pull request Jun 25, 2023

[Snyk] Fix for 28 vulnerabilities kissofire/node#102

Open

Uh oh!

Conversation

indutny commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

jasnell commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

trevnorris commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

jasnell commented Oct 13, 2015

Uh oh!

jasnell commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Oct 13, 2015

Uh oh!

indutny commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants