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

zlib: do not emit event on *Sync() methods #5707

Closed
wants to merge 5 commits into from
Closed

Conversation

@Trott
Copy link
Member

@Trott Trott commented Mar 15, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

zlib

Description of change

WIP right now, still need to do it for more methods than just
gunzipSync().

Needs a test, too.

Refs: #1668

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 15, 2016

Marking as semver-major due to the nature of the change... tho, I certainly hope no one had come to rely on this behavior ;-)

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 15, 2016

LGTM

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Mar 16, 2016

A citgm run, perhaps? cc @thealphanerd

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 16, 2016

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/116/

There has been some weirdness lately including

  • osx freezing due to infra related problem
  • moment failing on all ubuntu's due to upstream issue not yet fixed
  • periodic tape failures on various linux's (still researching why these are happening
@Trott
Copy link
Member Author

@Trott Trott commented Mar 16, 2016

@thealphanerd Is the failure in that CITGM run a false positive?

This is a work-in-progress so another run may be necessary later.

@Trott Trott added the wip label Mar 16, 2016
@Trott Trott force-pushed the Trott:inelegant branch Mar 16, 2016
@Trott
Copy link
Member Author

@Trott Trott commented Mar 16, 2016

Actually, it looks like I did in fact get all the instances I needed to get, so it may not need another CITGM run.

I do need to add a test though, for the every-day CI stuff.

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 16, 2016

So some of those failures on ppc are new. Glad everything else was green!!!

I'm running it again on master to see if that give us different results. Will add finding to this comment.

@Trott
Copy link
Member Author

@Trott Trott commented Mar 16, 2016

It seems like the only likely way someone might be tripped up by this change is if they are calling _processChunk() themselves. Maybe @ChALkeR is able to check to see if that's happening in npm-hosted modules. Either way, I support treating it as semver-major. But if there are modules that might break, I'd like to get PRs into them early...

@Trott
Copy link
Member Author

@Trott Trott commented Mar 17, 2016

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 17, 2016

Not seeing any of the ppc citgm problems on master specifically

  • watchify
  • gulp
  • vinyl-fs

Running citgm one more time to be sure. If vinyl-fs is having a regression then gulp would definitely be affected too

https://ci.nodejs.org/job/thealphanerd-smoker/118/

@Trott
Copy link
Member Author

@Trott Trott commented Mar 17, 2016

eslint is only failure this time. That's a known flaky?

@Trott
Copy link
Member Author

@Trott Trott commented Mar 17, 2016

Only CI failure is known-flaky test-dns. Discussion is here: #5554

Someone, please feel free to figure out how to fix that one. :-/

@Trott Trott removed the wip label Mar 17, 2016
@Trott
Copy link
Member Author

@Trott Trott commented Mar 17, 2016

Removing in progress label and now actively soliciting reviews and LGTMs.

@Trott Trott force-pushed the Trott:inelegant branch Mar 17, 2016
@Trott Trott mentioned this pull request Mar 17, 2016
2 of 4 tasks
require('../common');
const zlib = require('zlib');

const zipper = new zlib.Gzip();

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 17, 2016
Member

shouldn't this be testing zlib.gzipSync()?

This comment has been minimized.

@Trott

Trott Mar 17, 2016
Author Member

If it used zlib.gzipSync() then the object that emits the close event would not be accessible by the test.

So instead, this is emulating zlibBufferSync() and calling _processChunk() without a callback so that it can have access to the event if it fires.

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 17, 2016

Clean citgm run this time

@Trott
Copy link
Member Author

@Trott Trott commented Mar 18, 2016

Since @jasnell labeled this semver-major, that means it warrants some form of review by @nodejs/ctc. So, uh, @-mentioning CTC in the hopes a few people can look it over. I'm hoping this can land for Node 6.0 in April.

@@ -453,7 +453,9 @@ Zlib.prototype.flush = function(kind, callback) {
}
};

Zlib.prototype.close = function(callback) {
Zlib.prototype.close = function(callback, options) {
options = options || {};

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2016
Contributor

Object.assign() is safer than this pattern in the event that a non-object truthy value is passed. Not sure if we want to cover up those types of programming errors, but thought I'd at least mention it.

This comment has been minimized.

@Trott

Trott Mar 18, 2016
Author Member

Might as well do it right the first time. I'll make that change.

This comment has been minimized.

@Trott

Trott Mar 18, 2016
Author Member

OK, done. PTAL.

Zlib.prototype.close = function(callback) {
Zlib.prototype.close = function(callback, options) {
options = options || {};

if (callback)

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2016
Contributor

Would it work to detect the presence of a callback instead of a sync option?

This comment has been minimized.

@Trott

Trott Mar 18, 2016
Author Member

I don't think that will work unless we're OK breaking any existing userland code that looks like this:

'use strict';
const zlib = require('zlib');

const zipper = new zlib.Gzip();
zipper.on('close', () => { // do something });
zipper.close();

No idea how common that is. It's certainly atypical, as close() is undocumented, but it is exposed...

@Trott Trott force-pushed the Trott:inelegant branch Mar 18, 2016
zipper.on('close', () => { throw new Error('unexpected `close` event'); });

const buffer = new Buffer('hello world');
zipper._processChunk(buffer, zlib.Z_FINISH);

This comment has been minimized.

@trevnorris

trevnorris Mar 18, 2016
Contributor

For sake of paranoia, is there an event that would notify that the chunk has been processed and called before 'close' would be called? I'd like to assert that that event is called in process exit.

This comment has been minimized.

@Trott

Trott Mar 18, 2016
Author Member

@trevnorris I don't believe so, but would this more complete test address the concern?

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

const shouldNotBeCalled = () => { throw new Error('unexpected event'); };

const message = 'Come on, Fhqwhgads.';

const zipper = new zlib.Gzip();
zipper.on('close', shouldNotBeCalled);

const buffer = new Buffer(message);
const zipped = zipper._processChunk(buffer, zlib.Z_FINISH);

const unzipper = new zlib.Gunzip();
unzipper.on('close', shouldNotBeCalled);

const unzipped = unzipper._processChunk(zipped, zlib.Z_FINISH);
assert.notEqual(zipped.toString(), message);
assert.strictEqual(unzipped.toString(), message);

This comment has been minimized.

@Trott

Trott Mar 19, 2016
Author Member

You know, I think I like this more complete test regardless, so I'll push that up too.

@Trott Trott force-pushed the Trott:inelegant branch Mar 18, 2016
@jasnell
Copy link
Member

@jasnell jasnell commented Mar 19, 2016

fwiw, if anyone disagrees with the semver-major, feel free to downgrade it!

Trott added 3 commits Mar 15, 2016
WIP right now, still need to do it for more methods than just
gunzipSync().

Refs: #1668
@Trott Trott force-pushed the Trott:inelegant branch to 2a86be9 Mar 19, 2016
@Trott
Copy link
Member Author

@Trott Trott commented Mar 19, 2016

The more I think about this, the less happy I am with the options object. So I've refactored it out. PTAL.

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Mar 19, 2016

I'm not normally in zlib, but change looks clean. LGTM

@Trott
Copy link
Member Author

@Trott Trott commented Mar 19, 2016

fwiw, if anyone disagrees with the semver-major, feel free to downgrade it!

I'm all for "when in doubt, make it major", so I have no problem with it. This is doubly true considering that a semver major release is coming up in about a month, so if this lands relatively soon, it will be released in the not-too-distant future.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Mar 19, 2016

LGTM.

Speed and memory usage remain stable after this change with my test here: #1665 (comment).

Trott added a commit to Trott/io.js that referenced this pull request Mar 19, 2016
Asynchronous functions in `zlib` should not emit the close event.

This fixes an issue where asynchronous calls in a for loop could exhaust
memory because the pending event prevents the objects from being garbage
collected.

Fixes: nodejs#1668
PR-URL: nodejs#5707
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@Trott
Copy link
Member Author

@Trott Trott commented Mar 19, 2016

Landed in 8b43d3f

@Trott Trott closed this Mar 19, 2016
@Trott Trott added this to the 6.0.0 milestone Mar 22, 2016
@Trott Trott mentioned this pull request Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants