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

unpipe() and resume() in _transform() #31190

Closed
utftu opened this issue Jan 4, 2020 · 5 comments
Closed

unpipe() and resume() in _transform() #31190

utftu opened this issue Jan 4, 2020 · 5 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@utftu
Copy link

utftu commented Jan 4, 2020

  • Node.js Version: 13.*
  • OS: mac
  • Scope (install, code, runtime, meta, other?):
  • Module (and version) (if relevant):

In case of synchronous pipe removal in _transform, the resume method does not resume as expected

const stream = require('stream')

const fs = require('fs');
const readStream = fs.createReadStream('big.txt')
const writeStream = fs.createWriteStream('result.txt');

const transformStream = new class extends stream.Transform {

  _transform(chunk, encoding, callback) {
    readStream.unpipe()
    readStream.resume()
  }
}

readStream.on('end', () => {
  console.log('never print')
})


readStream
  .pipe(transformStream)
  .pipe(writeStream)

Nevertheless, resume operation during an asynchronous call and removal not from the _transform method

Can someone explain this behavior why unpipe () and resume () do not work in the _transform method?

@addaleax addaleax transferred this issue from nodejs/help Jan 5, 2020
@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Jan 5, 2020
@addaleax
Copy link
Member

addaleax commented Jan 5, 2020

/cc @ronag

@ronag

This comment has been minimized.

@ronag

This comment has been minimized.

@addaleax
Copy link
Member

addaleax commented Jan 5, 2020

@ronag The only two things I could think of are a) process.exit() or equivalents being called or b) blocking JS execution that prevents the nextTick queue from running.

For debugging Node.js core, it’s sometimes a good idea to use process._rawDebug() rather than console.log(), which doesn’t use streams under the hood.

That being said, if I just add the console.log() calls like you did, it does print output for me.

@ronag
Copy link
Member

ronag commented Jan 5, 2020

For debugging Node.js core, it’s sometimes a good idea to use process._rawDebug() rather than console.log()

Oh, thanks for that! I'll keep digging.

ronag added a commit to nxtedition/node that referenced this issue Jan 5, 2020
pipe() ondata should not control flow state if cleaned up.

Fixes: nodejs#31190
MylesBorins pushed a commit that referenced this issue Jan 16, 2020
pipe() ondata should not control flow state if cleaned up.

Fixes: #31190

PR-URL: #31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.

Fixes: nodejs#31190

Backport-PR-URL: nodejs#32264
PR-URL: nodejs#31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
ronag added a commit to nxtedition/node that referenced this issue Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.

Fixes: nodejs#31190

PR-URL: nodejs#31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: nodejs#32264
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.

Fixes: #31190

Backport-PR-URL: #32264
PR-URL: #31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #32264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants