Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Only apply drain listener when callback provided #25679

Closed
wants to merge 1 commit into from

Conversation

CraigCav
Copy link

When using the compression middleware in express with Server-Sent Events, I've noticed a possible EventEmitter memory leak detected. 11 drain listeners added. message appearing from Gzip.addListener:

at Gzip.addListener (events.js:179:15)
at Gzip.Readable.on (_stream_readable.js:671:33)
at Gzip.once (events.js:204:8)
at Gzip.Zlib.flush (zlib.js:451:10)
at ServerResponse.flush (/app/node_modules/compression/index.js:63:16)

The error can be reproduced using the SSE example on the expressjs/compression readme.

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.

Judging by line 449, an attempt has been made to avoid this situation, however, not all code paths have had the empty listener check applied.

@cjihrig
Copy link

cjihrig commented Jul 14, 2015

LGTM, although it would be better if the code actually checked that callback was a function instead of any truthy value.

@jasnell
Copy link
Member

jasnell commented Jul 14, 2015

hmm... according to the docs, callback is not optional, so I'm wondering if this is actually doing us a bit of a disservice. If the intent was to allow callback to be optional, then the docs need to be updated as well to reflect that. If the intent was for callback to be required, I'd rather see an explicit throw if callback is not a function.

@CraigCav
Copy link
Author

If the intent was for callback to be required, I'd rather see an explicit throw if callback is not a function.

Agreed, although I'd be concerned that this was a breaking change for anyone currently calling stream.flush().

In my specific case, requiring a callback would also mean that the possible EventEmitter memory leak detected. 11 drain listeners added. message would be unavoidable. That said, given that the frequent calling of flush is undesirable anyway (since it limits the effectiveness of the compression algorithm) my case maybe better resolved by disabling the compression middleware for SSE routes.

I can't speak for the original intent of the callback parameter. Given callback isn't called until the drain event is emitted (which doesn't seem to necessarily coincide with the completion of the flush), I'm not even sure I can provide a use case for having the callback parameter that wouldn't be better suited to having the caller add the listener themselves.

I'm happy to update the docs to reflect callback being optional if that is appropriate?

@jasnell
Copy link
Member

jasnell commented Aug 6, 2015

@joyent/node-tsc ... what do you think?

@jasnell jasnell added the events label Aug 6, 2015
this.once('drain', function() {
self.flush(callback);
});
if (callback) {
Copy link

Choose a reason for hiding this comment

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

If we're going to add this check, might as well make it a check for a function.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

@usernameisalreadytaken2014

I'm also running into this problem (node v4.2.1).

Also using flush() with SSE.

Disagree with @CraigCav that compression is not useful. My stream goes from 600 KiB down to 34 KiB with compression enabled on an SSE stream.

@CraigCav
Copy link
Author

@usernameisalreadytaken2014 I wasn't saying that compression is not useful. I was saying that in my specific circumstances, disabling the compression middleware for SSE routes might be an acceptable work-around. Disabling compression for all SSE routes is not going to be an appropriate solution for everyone.

I would much rather have the listener only applied if an optional callback is provided.

@usernameisalreadytaken2014

@CraigCav - check! I misunderstood, apologies.

Did you find a workaround? Does attaching a callback help?

(And if so, is it necessary to hold back further write()s until the callback has been activated, or?...)

@CraigCav
Copy link
Author

Does attaching a callback help?

I don't think so. The problem as I see it is that, regardless of whether a callback is provided, a drain listener is attached. Given that with flush is being called multiple times to push SSE's down to the client, multiple event handlers are being added.

I haven't found a workaround (other than removing compression which doesn't sound helpful for your case). It might be possible to remove all drain listeners after calling flush, although I haven't explored the side-effects of doing so.

@MylesBorins
Copy link

@CraigCav would it make sense to resubmit this PR against nodejs/node?

@Fishrock123
Copy link

@thealphanerd I was planning to, but my dev machine is broken for a couple days anyways. Feel free to take it if @CraigCav isn't around. :)

@MylesBorins
Copy link

The fix is now on master and v4.x-staging

@CraigCav CraigCav deleted the null-check-drain-listener branch November 24, 2015 22:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants