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

fs: fix leak of end listener in ReadStream #1509

Closed
wants to merge 1 commit into from
Closed

fs: fix leak of end listener in ReadStream #1509

wants to merge 1 commit into from

Conversation

coderaiser
Copy link
Contributor

When read is end, it leaves listener of end event.
This could be checked this way:

var fs      = require('fs'),
    read    = fs.createReadStream(__filename),
    write   = fs.createWriteStream(__filename + '-copy');

read.pipe(write);

read.once('end', function() {
    var listeners = read.listeners('end');
    console.log('' + listeners[0])
});

The code will log this function:

function () {
    if (this.autoClose) {
      this.destroy();
    }
  }

Version of iojs is:

> iojs -v
v1.8.1

@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Apr 23, 2015
@mscdex
Copy link
Contributor

mscdex commented Apr 23, 2015

Has this caused a problem for you? The stream is not re-usable after it ends, so there shouldn't be a need to manually remove listeners.

Additionally, using .once() is slower (currently) than .on() because the former creates a new closure every time.

@coderaiser
Copy link
Contributor Author

Additionally, using .once() is slower (currently) than .on() because the former creates a new closure every time.

Any way in fs once used 5 times and on used only once.

It's about leaking listener, why don't just remove it when it no need? It would simplify work for garbage collector and make code little bit cleaner.

I found this when wrote tests for pipe-io module. It simplifies work with streams. I checked that no listeners from this module is leaking to result streams, and found out that node listeners is leaking.

@brendanashworth
Copy link
Contributor

Sorry for letting this sit for so long. I think it makes sense - LGTM, even if its just theoretical. @mscdex do you have an opinion against this? (I think the .on() vs .once() is a good point, too)

@mscdex
Copy link
Contributor

mscdex commented May 23, 2015

@brendanashworth Something very similar to this was already discussed in #1510 and as far as I can tell it was decided it was a non-issue.

@brendanashworth
Copy link
Contributor

@mscdex wow, that's an impressive drawing. Thanks for enlightening me.

This is probably a non-issue too, then. I'll close. Thanks for the pull request, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants