stream: avoid pause with unpipe in buffered write

[mscdex]

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

Fixes: #2323

[LinusU]

@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]

/cc @nodejs/streams

[calvinmetcalf]

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

[mscdex]

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]

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]

Ok, updated PR according to suggestions.

[Fishrock123]

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

[mscdex]

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]

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

[mscdex]

Landed in 6899094.

[LinusU]

Nice, thanks!

[jasnell]

Landed in v4.x in 7b9f78a

[omrilitov]

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

[mscdex]

@omrilitov It's included in v4.2.0.