zlib: only apply drain listener if given callback #3534

MylesBorins · 2015-10-26T18:04:13Z

When stream.flush() is called without a callback, an empty listener is
being added. Since flush may be called multiple times to push SSE's
down to the client, multiple noop listeners are being added. This in
turn causes the memory leak detected message.

fixes #3529

This commit has been cherry picked from a pr on v0.x-archive
nodejs/node-v0.x-archive#25679

MylesBorins · 2015-10-26T18:05:21Z

I'm not sure what the official process is for grabbing commits from pr's from the archive.

I opted to cherry pick the commit and update the commit message, but retained the authorship with the original author.

Trott · 2015-10-26T18:06:05Z

Is there some way to add a test for this?

MylesBorins · 2015-10-26T18:07:32Z

most likely. I'm more than happy to try and take on some of that work, but would love to see if this would be an acceptable solution before putting in time... especially since I'm not 100% on exactly how to test it yet :P

bnoordhuis · 2015-10-26T20:24:21Z

lib/zlib.js

+    if (callback) {
+      this.once('drain', function() {
+        self.flush(callback);
+      });


I think I spot a bug here: the call should be self.flush(kind, callback), it forgets the kind now.

Also, I'd ES6 this up and write it as this.once('drain', () => this.flush(kind, callback)).

Fishrock123 · 2015-10-26T20:56:25Z

I'm not sure what the official process is for grabbing commits from pr's from the archive.

This seems about right. :)

MylesBorins · 2015-10-26T21:51:45Z

There were a few different things that have come in from the thread so I have split them up .

First was the bug that @bnoordhuis spotted where kind was not being to recursive calls of flush. This part should be able to be backported even if the rest of this PR is deemed semver breaking.

I also included a separate patch to explicitly check that callback is a function rather than just truthy. This was originally requested by @cjihrig in the original thread

This pattern does appear throughout the codebase in both ways... I'm going to make a separate issue about this.

It is also worth bringing up that @jasnell mentioned in the original thread that the zlib docs do not state that the callback is optional... so the docs may need to be updated.

MylesBorins · 2015-10-26T22:01:35Z

meta conversation about how callbacks are dealt with --> #3536

cjihrig · 2015-10-26T23:42:53Z

My personal opinion is that you should try to maintain the original author of the commit when cherry picking.

cjihrig · 2015-10-26T23:45:07Z

lib/zlib.js

@@ -438,16 +438,15 @@ Zlib.prototype.flush = function(kind, callback) {
  }

  if (ws.ended) {
-    if (callback)
+    if (typeof callback ==='function')


Can you add a space between === and 'function' throughout this PR.

bnoordhuis · 2015-10-27T11:19:24Z

Regression tests would be good.

My personal opinion is that you should try to maintain the original author of the commit when cherry picking.

I agree but 9d9f681 doesn't look like a cherry-pick to me.

MylesBorins · 2015-10-29T00:14:01Z

@bnoordhuis I could have sworn I responded to this already 😵

that commit does differ from the original, but only in as far as changes were requested in this thread... seems like a fine line to me, so I opted to be conservative here

MylesBorins · 2015-11-03T18:08:03Z

is the lack of a test the only thing holding this up atm?

bnoordhuis · 2015-11-03T18:10:11Z

Yes. I'd drop 0865370 for now because it makes it semver-major instead of just a bug fix.

MylesBorins · 2015-11-03T18:11:52Z

kk. I'll leave that to get cleaned up by #3539

Fishrock123 · 2015-11-05T16:11:06Z

@thealphanerd status of this?

MylesBorins · 2015-11-05T18:05:24Z

@Fishrock123 planning to work on this today actually. Just have not had time to figure out how to test the change yet.

bnoordhuis · 2015-11-10T22:16:15Z

@thealphanerd Status?

MylesBorins · 2015-11-11T18:31:09Z

Still trying to wrap my head around Zlib, specifically the streams bit. Will focus on this today and hopefully have something tangible

MylesBorins · 2015-11-11T23:13:47Z

I just pushed a first pass at doing a test.

an instance of deflator is created and forced into a drain state and flush is called.
It keeps track of the internal write stream of the zlib object, both when it needs to drain and once it has drained.
It assures that a drain event has been fired.
deflator.flush is shimmed to keep track of times it is executed, which is tested to ensure it has been executed twice

Now I'm not 100% that there is no way that calling flush right after write may have flush execute while it's internal write stream is not needing to drain. (Which would cause this test to fail)

If anyone has suggestions on how to improve this I am very interested. I would also be interested if anyone has a better practice for spying on functions as I did with the flush shim.

/cc @jasnell @bnoordhuis @chrisdickinson @Fishrock123

MylesBorins · 2015-11-11T23:18:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/707/

MylesBorins · 2015-11-12T00:20:50Z

The failing CI bits seem unrelated to this PR to me

bnoordhuis · 2015-11-13T14:28:40Z

test/parallel/test-zlib-flush-drain.js

+});
+
+deflater.on('drain', function() {
+  drained = true;


If I understand the logic of this test right, I'd turn this into a counter and check that it's equal to flushCount on exit. If I don't understand the logic right, I'd still turn it into a counter and check that it has the expected value. :-)

drain should only execute once if I recall correctly, but I can double check that

ok I've checked, and drain is only emitted once (as expected). That being said I will make it a counter rather than checking a boolean state

this is now fixed

I'm still seeing a boolean (or again? I could swear you'd made the changes.)

jasnell · 2015-11-13T21:39:50Z

LGTM

bnoordhuis · 2015-11-13T21:43:37Z

LGTM but can you run the CI again and would you mind fixing the spelling of my GH handle in the first commit?

MylesBorins · 2015-11-13T21:48:28Z

Commit updated

new ci --> https://ci.nodejs.org/job/node-test-pull-request/722/

I'm more than happy to land this after the CI run, should this all be flattened to a single commit or is it alright to land as three?

@bnoordhuis

Bug spotted by @bnoordhuis while doing code review on #3534 #3534 (comment)

When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message.

@bnoordhuis

Bug spotted by @bnoordhuis while doing code review on #3534 Refs: #3534 (comment) PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test assures that if flush is called while the zlib object needs to be drained that it will defer the callback until after the drain. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

@bnoordhuis

Bug spotted by @bnoordhuis while doing code review on #3534 Refs: #3534 (comment) PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test assures that if flush is called while the zlib object needs to be drained that it will defer the callback until after the drain. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

@bnoordhuis

Bug spotted by @bnoordhuis while doing code review on #3534 Refs: #3534 (comment) PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test assures that if flush is called while the zlib object needs to be drained that it will defer the callback until after the drain. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

@bnoordhuis

Bug spotted by @bnoordhuis while doing code review on #3534 Refs: #3534 (comment) PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test assures that if flush is called while the zlib object needs to be drained that it will defer the callback until after the drain. PR-URL: #3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

bnoordhuis reviewed Oct 26, 2015
View reviewed changes

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Oct 26, 2015

cjihrig reviewed Oct 26, 2015
View reviewed changes

MylesBorins added the lts-watch-v4.x label Nov 11, 2015

jasnell mentioned this pull request Nov 11, 2015

zlib: inconsistent call to flush callback on oversized buffers #3782

Closed

bnoordhuis reviewed Nov 13, 2015
View reviewed changes

Myles Borins and others added 2 commits November 13, 2015 14:19

zlib: pass kind to recursive calls to flush

ce8a3e1

Bug spotted by @bnoordhuis while doing code review on #3534 #3534 (comment)

MylesBorins removed the lts-watch-v4.x label Nov 16, 2015

rvagg mentioned this pull request Nov 17, 2015

v4.2.2+1 commit candidates nodejs/Release#57

Closed

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

This was referenced Jul 27, 2017

zlib flush() fails to flush all data when callback is not provided and needDrain == true #14523

Closed

zlib: fix interaction of flushing and needDrain #14527

Closed

aliscco mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1233

Open

aliscco mentioned this pull request Mar 23, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1267

Open

aliscco mentioned this pull request May 14, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1437

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: only apply drain listener if given callback #3534

zlib: only apply drain listener if given callback #3534

MylesBorins commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

Trott commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

bnoordhuis Oct 26, 2015

MylesBorins Oct 26, 2015

Fishrock123 commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

cjihrig commented Oct 26, 2015

cjihrig Oct 26, 2015

MylesBorins Oct 26, 2015

bnoordhuis commented Oct 27, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Nov 3, 2015

bnoordhuis commented Nov 3, 2015

MylesBorins commented Nov 3, 2015

Fishrock123 commented Nov 5, 2015

MylesBorins commented Nov 5, 2015

bnoordhuis commented Nov 10, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 12, 2015

bnoordhuis Nov 13, 2015

MylesBorins Nov 13, 2015

MylesBorins Nov 13, 2015

MylesBorins Nov 13, 2015

bnoordhuis Nov 13, 2015

jasnell commented Nov 13, 2015

bnoordhuis commented Nov 13, 2015

MylesBorins commented Nov 13, 2015

zlib: only apply drain listener if given callback #3534

zlib: only apply drain listener if given callback #3534

Conversation

MylesBorins commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

Trott commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

MylesBorins commented Oct 26, 2015

cjihrig commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Oct 27, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Nov 3, 2015

bnoordhuis commented Nov 3, 2015

MylesBorins commented Nov 3, 2015

Fishrock123 commented Nov 5, 2015

MylesBorins commented Nov 5, 2015

bnoordhuis commented Nov 10, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 11, 2015

MylesBorins commented Nov 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 13, 2015

bnoordhuis commented Nov 13, 2015

MylesBorins commented Nov 13, 2015