stream: avoid pause with unpipe in buffered write #2325

mscdex · 2015-08-07T14:36:05Z

If a pipe is cleaned up (due to unpipe) during a failed write(),
the source stream can get stuck in a paused state.

LinusU · 2015-08-14T23:03:47Z

@mscdex This fixes the problem, thank you so much, would love to see this landed 💯 👍

I'm currently using this as a workaround. Is it stupid as fuck or should it work?

function done (err) {
  if (isDone) return
  isDone = true

  req.unpipe(busboy)
  req.resume()
  busboy.removeAllListeners()

  /* Work around bug in Node.js
   * https://github.com/nodejs/node/issues/2323
   */
  setImmediate(function () {
    if (req._readableState.awaitDrain === 1) {
      req._readableState.awaitDrain--
      req.resume()
    }
  })

  onFinished(req, function () { next(err) })
}

I'm also having some trouble which forces me to add the onFinished part, it seems like I have to wait for all data to arrive before I can send data back... I'll try to investigate and open another issue.

Again, thank you for the quick help!

mscdex · 2015-08-15T02:13:55Z

/cc @nodejs/streams

calvinmetcalf · 2015-08-15T17:36:48Z

test/parallel/test-stream-pipe-cleanup-pause.js

+writer1.once('chunk-received', function() {
+  reader.unpipe(writer1);
+  reader.pipe(writer2);
+  reader.push(new Buffer(560000));


what's the significants of this number, is it being used to bust the buffer?

It was copied from the test case in #2323. I think the point though is to fill up the internal buffer to cause the write() to return false.

LinusU · 2015-08-29T22:19:52Z

If someone could help me to find a workaround for earlier Node versions (preferably back to 0.10) that would be very much appreciated.

I've tried here: expressjs/multer#205

mscdex · 2015-09-10T16:20:48Z

Ok, updated PR according to suggestions.

Fishrock123 · 2015-09-14T13:59:39Z

Ci: https://ci.nodejs.org/job/node-test-pull-request/296/

mscdex · 2015-10-11T16:41:31Z

updated to use common.mustCall() in the test.

CI: https://ci.nodejs.org/job/node-test-commit/799/
Looks like two of the Windows machines failed for different reasons. Not sure what's up with that.

Does this look good to land now?

thefourtheye · 2015-10-11T16:45:23Z

There can be a comment which explains the importance of 56000 or link to this PR. LGTM.

If a pipe is cleaned up (due to unpipe) during a write that returned false, the source stream can get stuck in a paused state. Fixes: nodejs#2323

If a pipe is cleaned up (due to unpipe) during a write that returned false, the source stream can get stuck in a paused state. Fixes: #2323 PR-URL: #2325 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

mscdex · 2015-10-11T18:09:19Z

Landed in 6899094.

If a pipe is cleaned up (due to unpipe) during a write that returned false, the source stream can get stuck in a paused state. Fixes: nodejs#2323 PR-URL: nodejs#2325 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

LinusU · 2015-10-12T07:25:54Z

Nice, thanks!

If a pipe is cleaned up (due to unpipe) during a write that returned false, the source stream can get stuck in a paused state. Fixes: #2323 PR-URL: #2325 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell · 2015-10-12T13:43:03Z

Landed in v4.x in 7b9f78a

omrilitov · 2015-10-26T12:25:27Z

Is this fixed in v4.2.0?
It's listed on the commits but it got merged after 4.2.0 release

mscdex · 2015-10-26T14:26:52Z

@omrilitov It's included in v4.2.0.

In 6899094 (nodejs#2325), the conditions for increasing `readableState.awaitDrain` when writing to a piping destination returns false were changed so that they could not actually be met, effectively leaving `readableState.awaitDrain` with a constant value of 0. This patch changes the conditions to testing whether the stream for which `.write()` returned false is still a piping destination, which was likely the intention of the original patch. Fixes: nodejs#5820

In 6899094 (#2325), the conditions for increasing `readableState.awaitDrain` when writing to a piping destination returns false were changed so that they could not actually be met, effectively leaving `readableState.awaitDrain` with a constant value of 0. This patch changes the conditions to testing whether the stream for which `.write()` returned false is still a piping destination, which was likely the intention of the original patch. Fixes: #5820 Fixes: #5257 PR-URL: #6023 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mscdex mentioned this pull request Aug 7, 2015

Redirecting from socket hangs with large data #2323

Closed

mscdex added the stream Issues and PRs related to the stream subsystem. label Aug 7, 2015

LinusU mentioned this pull request Aug 14, 2015

lib: work around node.js bug expressjs/multer#205

Merged

calvinmetcalf reviewed Aug 15, 2015
View reviewed changes

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

mscdex force-pushed the fix-stream-regression branch from 37b7ff1 to bcc79eb Compare September 10, 2015 16:19

mscdex force-pushed the fix-stream-regression branch 2 times, most recently from 9b13ecb to b879ce0 Compare October 11, 2015 16:03

mscdex force-pushed the fix-stream-regression branch from b879ce0 to 6176c8b Compare October 11, 2015 16:56

stream: avoid pause with unpipe in buffered write

91819e0

If a pipe is cleaned up (due to unpipe) during a write that returned false, the source stream can get stuck in a paused state. Fixes: nodejs#2323

mscdex force-pushed the fix-stream-regression branch from 6176c8b to 91819e0 Compare October 11, 2015 18:06

mscdex closed this Oct 11, 2015

jasnell added land-on-v4.x labels Oct 12, 2015

mscdex changed the title ~~stream: avoid pause with unpipe in failed write~~ stream: avoid pause with unpipe in buffered write Oct 26, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

mscdex deleted the fix-stream-regression branch December 17, 2015 18:20

Fishrock123 mentioned this pull request Mar 21, 2016

Piping readable stream to multiple writeable streams results in crash #5820

Closed

addaleax mentioned this pull request Apr 2, 2016

stream: Fix readableState.awaitDrain mechanism #6023

Closed

4 tasks

addaleax mentioned this pull request Jun 5, 2016

stream: reset awaitDrain after manual .resume() #7160

Closed

3 tasks

This was referenced Jun 13, 2016

Streams Regression in v4.x nodejs/Release#112

Closed

stream: only increase awaitDrain once for each pipe destination #7292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: avoid pause with unpipe in buffered write #2325

stream: avoid pause with unpipe in buffered write #2325

mscdex commented Aug 7, 2015

LinusU commented Aug 14, 2015

mscdex commented Aug 15, 2015

calvinmetcalf Aug 15, 2015

mscdex Aug 15, 2015

LinusU commented Aug 29, 2015

mscdex commented Sep 10, 2015

Fishrock123 commented Sep 14, 2015

mscdex commented Oct 11, 2015

thefourtheye commented Oct 11, 2015

mscdex commented Oct 11, 2015

LinusU commented Oct 12, 2015

jasnell commented Oct 12, 2015

omrilitov commented Oct 26, 2015

mscdex commented Oct 26, 2015

stream: avoid pause with unpipe in buffered write #2325

stream: avoid pause with unpipe in buffered write #2325

Conversation

mscdex commented Aug 7, 2015

LinusU commented Aug 14, 2015

mscdex commented Aug 15, 2015

calvinmetcalf Aug 15, 2015

Choose a reason for hiding this comment

mscdex Aug 15, 2015

Choose a reason for hiding this comment

LinusU commented Aug 29, 2015

mscdex commented Sep 10, 2015

Fishrock123 commented Sep 14, 2015

mscdex commented Oct 11, 2015

thefourtheye commented Oct 11, 2015

mscdex commented Oct 11, 2015

LinusU commented Oct 12, 2015

jasnell commented Oct 12, 2015

omrilitov commented Oct 26, 2015

mscdex commented Oct 26, 2015