Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: fix stalled pipeline bug (v3.x backport) #3558

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 28, 2015

Backport of #3342 / ab03635, only including _http_outgoing.js changes as per @indutny's suggestion. cherry-pick wasn't clean so doing this as a new commit.

/R=@indutny

Original commit ab03635:

  This is a two-part fix:

  - Fix pending data notification in `OutgoingMessage` to notify server
    about flushed data too
  - Fix pause/resume behavior for the consumed socket. `resume` event is
    emitted on a next tick, and `socket._paused` can already be `true` at
    this time. Pause the socket again to avoid PAUSED error on parser.

  Fix: https://github.com/nodejs/node/issues/3332
  PR-URL: https://github.com/nodejs/node/pull/3342
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@rvagg rvagg force-pushed the http-stalled-pipeline-backport-v3.x branch from 1937e6f to 241ad53 Compare October 28, 2015 05:56
Original commit ab03635:

  This is a two-part fix:

  - Fix pending data notification in `OutgoingMessage` to notify server
    about flushed data too
  - Fix pause/resume behavior for the consumed socket. `resume` event is
    emitted on a next tick, and `socket._paused` can already be `true` at
    this time. Pause the socket again to avoid PAUSED error on parser.

  Fix: nodejs#3332
  PR-URL: nodejs#3342
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@rvagg rvagg force-pushed the http-stalled-pipeline-backport-v3.x branch from 241ad53 to 2202774 Compare October 28, 2015 05:56
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Oct 28, 2015
@indutny
Copy link
Member

indutny commented Oct 28, 2015

LGTM

@rvagg
Copy link
Member Author

rvagg commented Oct 28, 2015

@indutny included test fails:

=== release test-http-pipeline-regr-3332 ===
Path: parallel/test-http-pipeline-regr-3332
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: 32 == 10000
    at process.<anonymous> (/Users/rvagg/git/iojs/io.js/test/parallel/test-http-pipeline-regr-3332.js:40:10)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)
Command: out/Release/iojs /Users/rvagg/git/iojs/io.js/test/parallel/test-http-pipeline-regr-3332.js

is it expected that it would pass when included as is?

@ChALkeR ChALkeR added lts Issues and PRs related to Long Term Support releases. and removed lts Issues and PRs related to Long Term Support releases. labels Oct 28, 2015
@indutny
Copy link
Member

indutny commented Oct 28, 2015

Checking

@indutny
Copy link
Member

indutny commented Oct 28, 2015

Wait, wut?! I didn't know that the parser consumption stuff was in v3... Sorry @rvagg, but the full patch should be ported then!

@rvagg
Copy link
Member Author

rvagg commented Jan 27, 2016

closing, as per #3465 (comment)

@rvagg rvagg closed this Jan 27, 2016
@rvagg rvagg deleted the http-stalled-pipeline-backport-v3.x branch January 27, 2016 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants