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

stream: another missing end event #18484

Closed
mafintosh opened this issue Jan 31, 2018 · 4 comments
Closed

stream: another missing end event #18484

mafintosh opened this issue Jan 31, 2018 · 4 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@mafintosh
Copy link
Member

mafintosh commented Jan 31, 2018

  • Version: master 3124146
  • Platform: all
  • Subsystem: stream

1e0f331 introduced another issue where end isn't emitted. Note that this is a different issue than #18294 although they seem related.

Here is a test case. After some poking around it seems to be back pressure related.

var stream = require('stream')

var ended = false
var missing = 50

var rs = new stream.Readable({
  objectMode: true,
  read: () => {
    if (missing--) rs.push({})
    else rs.push(null)
  }
})

var a = rs.pipe(new stream.PassThrough({objectMode: true}))
  .pipe(new stream.PassThrough({objectMode: true}))

a.on('end', function () {
  wrap.push(null)
})

var wrap = new stream.Readable({
  objectMode: true,
  read: () => {
    process.nextTick(function () {
      var data = a.read()
      if (data === null) {
        a.once('readable', function () {
          data = a.read()
          if (data !== null) wrap.push(data)
        })
      } else {
        wrap.push(data)
      }
    })
  }
})

wrap.resume()
wrap.on('end', function () {
  ended = true
})

process.on('exit', function () {
  if (!ended) throw new Error('stream should end')
})
@mafintosh
Copy link
Member Author

/cc @mcollina @AndreasMadsen

@mkartashov
Copy link

could this be somehow related? #18058 I.e. false positives "ends" vs no "ends"?..

@mcollina
Copy link
Member

mcollina commented Feb 1, 2018

@mkartashov no, this is definitely something appearing in master. I'll have a look into the other one.

@mafintosh
Copy link
Member Author

mafintosh commented Feb 1, 2018

I managed to boil this down to the following test case

var stream = require('stream')

var ticks = 17
var rs = new stream.Readable({
  objectMode: true,
  read: (s, n) => {
    if (ticks-- > 0) return process.nextTick(_ => rs.push({}))
    rs.push({})
    rs.push(null)
  }
})

var ws = new stream.Writable({
  highWaterMark: 0,
  objectMode: true,
  write: function (data, end, cb) {
    console.log('writing data', data)
    setImmediate(cb)
  }
})

var finished = false
rs.pipe(ws)

ws.on('finish', function () {
  finished = true
})

process.on('exit', function () {
  if (!finished) throw new Error('should have finished')
})

@Fishrock123 Fishrock123 added stream Issues and PRs related to the stream subsystem. confirmed-bug Issues with confirmed bugs. labels Feb 2, 2018
addaleax added a commit to addaleax/node that referenced this issue Feb 6, 2018
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.

By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).

PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit to addaleax/node that referenced this issue Feb 27, 2018
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.

By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).

PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.

By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).

PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants