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

.on('close') for a readable stream stops working when going from node 12.14.0 to 12.16.1 #32023

Closed
JamesSingleton opened this issue Feb 29, 2020 · 2 comments
Labels
zlib Issues and PRs related to the zlib subsystem.

Comments

@JamesSingleton
Copy link

JamesSingleton commented Feb 29, 2020

  • Version: 12.16.1
  • Platform: Windows (64 bit) and MacOS
  • Subsystem:

What steps will reproduce the bug?

The entire script can be found here (https://github.com/americanexpress/one-app/blob/master/scripts/dangers/bundle-sizes.js)

  1. Run the following code on Node 12.14.0 and below.
function getGzipSize(filePath) {
  let bytesWritten = 0;
  const byteCounter = new Writable({
    write(chunk, encoding, callback) {
      // we control the input stream, will always be a Buffer
      bytesWritten += chunk.length;
      callback();
    },
  });
  return new Promise((res, rej) => {
    fs.createReadStream(filePath)
      .pipe(zlib.createGzip({ level: 9 }))
      // writable streams do not fire the 'close' event anymore
      // https://github.com/nodejs/node/issues/21122#issuecomment-414622251
      .on('close', () => res(bytesWritten))
      .pipe(byteCounter)
      .on('error', rej);
  });
}
  1. Switch to Node 12.16.1

How often does it reproduce? Is there a required condition?

Consistently.

What is the expected behavior?

With this certain piece of code, on close of the stream, a markdown file should be printed.

What do you see instead?

Nothing is printed due to it not closing.

Additional information

Switching close to end makes it work. Also, this comes from a discussion I had with @mcollina and @jasnell on Twitter.

@Hakerh400
Copy link
Contributor

bisected to 7cad756

@Trott
Copy link
Member

Trott commented Mar 1, 2020

bisected to 7cad756

@addaleax

@addaleax addaleax added the zlib Issues and PRs related to the zlib subsystem. label Mar 1, 2020
tadjik1 pushed a commit to tadjik1/node that referenced this issue Mar 2, 2020
Call the close method after readable 'end' so that 'close' will be
emitted afterwards.

Fixes: nodejs#32023
MylesBorins pushed a commit that referenced this issue Mar 11, 2020
Call the close method after readable 'end' so that 'close' will be
emitted afterwards.

Fixes: #32023

PR-URL: #32050
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Mar 11, 2020
Call the close method after readable 'end' so that 'close' will be
emitted afterwards.

Fixes: nodejs#32023

PR-URL: nodejs#32050
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ronag pushed a commit to nxtedition/node that referenced this issue Mar 11, 2020
Call the close method after readable 'end' so that 'close' will be
emitted afterwards.

Fixes: nodejs#32023

PR-URL: nodejs#32050
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants