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

[v13.x] zlib: emits 'close' event after readable 'end' #32215

Closed

Conversation

MylesBorins
Copy link
Contributor

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]

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]>
@nodejs-github-bot nodejs-github-bot added v13.x zlib Issues and PRs related to the zlib subsystem. labels Mar 11, 2020
@nodejs-github-bot
Copy link
Collaborator

@MylesBorins MylesBorins requested a review from addaleax March 11, 2020 20:56
@MylesBorins
Copy link
Contributor Author

hey @addaleax and @tadjik1

you can see from the above CI run that this commit unfortunately breaks 13.x. Any idea what's up?

@ronag
Copy link
Member

ronag commented Mar 11, 2020

Seems to fail with the following:

  ['createDeflate', 'createInflate', true],
  ['createDeflateRaw', 'createInflateRaw', true],
  ['createBrotliCompress', 'createBrotliDecompress', true]

Maybe related to the "extra data after the compressed data"?

@ronag
Copy link
Member

ronag commented Mar 11, 2020

I think I managed to reduce it to minimal repro:

'use strict';
require('../common');
const zlib = require('zlib');

function createWriter(target, buffer) {
  const writer = { size: 0 };
  const write = () => {
    target.write(Buffer.from([buffer[writer.size++]]), () => {
      if (writer.size < buffer.length) {
        target.flush(write);
      } else {
        target.end();
      }
    });
  };
  write();
  return writer;
}

const compData = Buffer.from('789c4a04000000ffff4a02000000ffff4a06000000ffff4a01000000ffff4a05000000ffff4a03000000ffff4a07000000ffffca00000000ffffca04000000ffffca02000000ffffca06000000ffffca01000000ffffca05000000ffffca03000000ffffca07000000ffff2a00000000ffff2a04000000ffff2a02000000ffff2a06000000ffff2a01000000ffff2a05000000ffff2a03000000ffff2a07000000ffffaa00000000ffffaa04000000ffffaa02000000ffff4a04000000ffff4a02000000ffff4a06000000ffff4a01000000ffff4a05000000ffff4a03000000ffff4a07000000ffffca00000000ffffca04000000ffffca02000000ffffca06000000ffffca01000000ffffca05000000ffffca03000000ffffca07000000ffff2a00000000ffff2a04000000ffff2a02000000ffff2a06000000ffff2a01000000ffff2a05000000ffff2a03000000ffff2a07000000ffffaa00000000ffffaa04000000ffffab02004250163f', 'hex')
const compDataExtra = Buffer.concat([compData, Buffer.from('extra')]);
const decomp = zlib.createInflate();
decomp.resume();
createWriter(decomp, compDataExtra);

@ronag
Copy link
Member

ronag commented Mar 11, 2020

@MylesBorins if 0e89b64 is also included this does not seem to fail.

@MylesBorins
Copy link
Contributor Author

@ronag that is strange as that change landed on 13.x as 31bbae7 and was released in 13.6 (it is also on 12)

@MylesBorins MylesBorins changed the title zlib: emits 'close' event after readable 'end' [v13.x] zlib: emits 'close' event after readable 'end' Mar 11, 2020
@ronag
Copy link
Member

ronag commented Mar 11, 2020

Yea sorry, my mistake. Re-applying it basically reverts this commit.

@ronag
Copy link
Member

ronag commented Mar 11, 2020

#32216 seems to resolve it on 13.x. I'm unsure why this doesn't fail on master.

@@ -274,7 +274,7 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) {
this._defaultFlushFlag = flush;
this._finishFlushFlag = finishFlush;
this._defaultFullFlushFlag = fullFlush;
this.once('end', _close.bind(null, this));
this.once('end', this.close);
Copy link
Member

@ronag ronag Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, just remove the event listener and enable autoDestroy: true. Which will ensure the stream is not prematurely destroyed.

@ronag
Copy link
Member

ronag commented Mar 14, 2020

@MylesBorins: I would recommend closing this in favor of #32220 and backporting that once it lands.

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 this pull request may close these issues.

3 participants