-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Labels
Comments
could this be somehow related? #18058 I.e. false positives "ends" vs no "ends"?.. |
@mkartashov no, this is definitely something appearing in master. I'll have a look into the other one. |
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')
}) |
3 tasks
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
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.
The text was updated successfully, but these errors were encountered: