Leak of "open" and "finish" listeners of writeStream on error

[coderaiser]

Listeners do not removed from writeStream when error occurs in iojs v1.8.1.

var fs = require('fs'),
 read = fs.createReadStream(__filename);
 write = fs.createWriteStream('/');

read.pipe(write);

write.on('error', function(error) {
 console.log(error)
 console.log('' + write.listeners('open')[0]);
 console.log('' + write.listeners('finish')[0]);
})

Result on Unix (without sudo):

{ [Error: EISDIR: illegal operation on a directory, open '/'] errno: -21, code: 'EISDIR', syscall: 'open', path: '/' }
function g() {
 this.removeListener(type, g);

 if (!fired) {
 fired = true;
 listener.apply(this, arguments);
 }
 }
function g() {
 this.removeListener(type, g);

 if (!fired) {
 fired = true;
 listener.apply(this, arguments);
 }
 }

[mscdex]

The stream is not usable after an error, so for most use cases the stream will be garbage collected after an error. Technically you could keep a reference to the stream around after an error, preventing GC, but why?

[chrisdickinson]

I lean towards "it's fine to leave the listeners attached" since, as @mscdex points out, the only way they create a leak is if client code is holding onto the stream itself; thus they represent a leak that depends on the user creating a leak themselves.

While I like the idea of removing all listeners from a stream once it becomes "inert," it'd be a mostly ideological change that would probably break a non-trivial amount of existing code (especially since folks subclass streams and treat them as generic event emitters!)

[coderaiser]

Why don't protect people from trapping on such leaking?
This behavior is not documented and it's could be unexpected that listeners are still on a stream after error event.

[meandmycode]

I feel like there's some confusion about this, what do you think is leaking here? the callback isn't holding the stream from being GC'd as the only thing referencing the function is the stream itself. If you are worried the stream is pinning a function in memory then you are seriously micro optimizing.

[chrisdickinson]

What follows is an attempt to illustrate why retaining the "finish" and "open" listeners after stream exhaustion is okay – apologies if it is a bit overwrought! Given the program:

var fs = require('fs'),
 read = fs.createReadStream(__filename);
 write = fs.createWriteStream('/');

read.pipe(write);

write.on('error', function(error) {
 console.log(error)
 console.log('' + write.listeners('open')[0]);
 console.log('' + write.listeners('finish')[0]);
})

We can draw a directed graph representing the objects in memory and their
relation to one another. Vertices will represent values – objects, functions,
primitives, and scopes – and directed edges will represent references.
References may be named (if they represent a property) or unnamed (if they
represent a private relationship.)

Certain objects in this graph are persistent – that is, they and all properties
flowing from them are protected from the garbage collector. If an object cannot
be reached by such an object, then it is subject to GC. All async APIs make their
callbacks persistent until they've been called; this protects those callbacks from
being GC'd while io.js does its work. Another important GC root is the global object,
which will not be GC'd until the program exits entirely.

If we halt execution after line 8, this is the (incomplete) object graph:

 [[global]] "console"
 ↑ \_______
 | \ "log"
 | Object ----→ Function
 [[scope]] 
 | \_____________ "require"
 ⅄ \
 ________/|\_________ ↓
 / "fs" | \ Function
 ↓ | |
 Object | "read" | "write"
 ↓ ↓
 Readable Writable

We'll add the open and finish listeners to the illustration:

 [[global]] "console"
 ↑ \_______
 | \ "log"
 | Object ----→ Function
 [[scope]] 
 | \_____________ "require"
 ⅄ \
 ________/|\_________ ↓
 / "fs" | \ Function
 ↓ | |
 Object | "read" | "write"
 ↓ ↓
 Readable Writable
 |
 | "_listeners"
 ↓
 Object
 |
 "open" _____⅄_____ "finish"
 / \
 ↓ ↓
 Function Function

And then the scopes they close over:

 _____________________________
 / \
 ↓ |
 [[global]] "console" |
 ↑ \_______ |
 | \ "log" |
 | Object ----→ Function |
 [[scope]] |
 | \_____________ "require" |
 ⅄ \ |
 ________/|\_________ ↓ |
 / "fs" | \ Function |
 ↓ | | |
 Object | "read" | "write" |
 ↑ ↓ ↓ |
 | Readable Writable |
 | ↑ | |
 ____________/ __/ | "_listeners" |
/ / ↓ |
| __________________/ Object |
| / | |
| | "open" _____⅄_____ "finish" |
| | / \ |
| | "self" ↓ ↓ |
| | Function Function \____________________________
| | | \___________ \
| | ↓ \ |
| [[scope]] ←-------- [[scope]] ↓ "path" |
| | [[scope]] ----→ String("/") |
| ⅄ / \____ |
| / \_________________________________________ ___/ \ "options" |
| | \ / ↓ |
| ⅄ Y undefined |
| / \__________________________ | |
| | \ \ ↓ |
| ↓ "data" ↓ "encoding" ↓ "cb" [[scope]] ____________________________/
| ??? ??? ??? / Y
| ___________/ |
 \_____________________________ / "module" ↓
 "exports" \ ↓ variables of fs.js
 Object

Let's look at the inner region, representing the writable stream and the open and
finish listeners – the one's we're worried about leaking.

 _____________________________
 / \
 ↓ |
 [[global]] "console" |
 ↑ \_______ |
 | \ "log" |
 | Object ----→ Function |
 [[scope]] |
 | \_____________ "require" |
 ⅄ \ |
 ________/|\_________ ↓ |
 / "fs" | \ Function |
 ↓ | ░░|░░ |
 Object | "read" ░░ | "write" |
 ↑ ↓ ░░░ ↓ ░░░░░░ |
 | Readable ░░ Writable ░░░░░ |
 | ░░░░░ ↑ | ░░░ |
 ____________/ ░░░░░░░░ __/ | "_listeners" ░░░ |
/ ░░░░░░░░░░░░░░░░░░ / ↓ ░░ |
| ░░ __________________/ Object ░░ |
| ░░░░ / | ░░ |
| ░░ | "open" _____⅄_____ "finish" ░░ |
| ░░ | / \ ░░ |
| ░░ | "self" ↓ ↓ ░░ |
| ░░ | Function Function ░░░░\____________________________
| ░░ | | \___________░░░░░░░░░░░░░░░░░░░ \
| ░░ | ↓ \ ░░░░░░░░░░ |
| ░░ [[scope]] ←-------- [[scope]] ↓ "path" ░░ |
| ░░ | [[scope]] ----→ String("/") ░░ |
| ░░ ⅄ / \____ ░░ |
| ░░ / \_________________________________________ ___/ \ "options" ░░ |
| ░░ | \ / ↓ ░░░ |
| ░░ ⅄ Y undefined ░░░░ |
| ░░ / \__________________________ ░░░░░|░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |
| ░░ | \ \ ░░░░░ ↓ |
| ░░ ↓ "data" ↓ "encoding" ↓ "cb" ░░░░ [[scope]] ____________________________/
| ░░??? ??? ░░░???░░░░░░░ / Y
| ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ___________/ |
 \_____________________________ / "module" ↓
 "exports" \ ↓ variables of fs.js
 Object

The edges crossing the boundary of this region are, starting at the top and going
clockwise:

1. From the scope of the example's module wrapper to the writable object ("write"),
2. and two edges leaving the region at the bottom, which represent the scopes
   of the listener functions, referring to their parent scope.

Note that there's only one arrow entering this region, the one named "write."
That means that the only thing retaining this section of the object graph is
our variable, "write." Once "write" is unreachable, or no longer points to this
region, the entire region can (and will eventually) be garbage collected.

If we continue execution, the region is expanded.

 _____________________________
 / \
 ↓ |
 [[global]] "console" |
 ↑ \_______ |
 ░░░|░░░ \ "log" |
 ░░░░░░ | ░░░░░ Object ----→ Function |
 ░░ [[scope]] ░░░░░░░░░░░░░░░░░░░░░░░ |
 ░░░ | \_____________ "require" ░░ |
 ░░ ⅄ \ ░░ |
 ░░________/|\_________ ↓ ░░ |
 /░"fs" | \ Function ░░ |
 ↓ ░░░ | | ░░ |
 Object ░░ | "read" | "write" ░░ |
 ↑ ░░ ↓ ↓ ░░ |
 | ░░ Readable Writable ░░ |
 | ░░░ ↑ | ░░░ |
 ____________/ ░░ __/ | "_listeners" ░░░ |
/ ░░░░░░░░░░░░░░░░ / ↓ ░░ |
| ░░ __________________/ Object ░░ |
| ░░░░ / | ░░ |
| ░░ | "open" _____⅄_____ "finish" ░░ |
| ░░ | / \ ░░ |
| ░░ | "self" ↓ ↓ ░░ |
| ░░ | Function Function ░░░░\____________________________
| ░░ | | \___________░░░░░░░░░░░░░░░░░░░ \
| ░░ | ↓ \ ░░░░░░░░░░ |
| ░░ [[scope]] ←-------- [[scope]] ↓ "path" ░░ |
| ░░ | [[scope]] ----→ String("/") ░░ |
| ░░ ⅄ / \____ ░░ |
| ░░ / \_________________________________________ ___/ \ "options" ░░ |
| ░░ | \ / ↓ ░░ |
| ░░ ⅄ Y undefined ░░ |
| ░░ / \__________________________ | ░░ |
| ░░ | \ \ ↓ ░░ |
| ░░ ↓ "data" ↓ "encoding" ↓ "cb" [[scope]] ____________________________/
| ░░??? ??? ??? / Y ░░
| ░░ ___________/ | ░░░░░
 \_____________________________ / "module" ↓ ░░░░░░░
 ░░"exports" \ ↓ variables of fs.js ░░░░░░
 ░░ Object ░░░░░
 ░░ ░░░░░░
 ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

Consider the edges that flow across the bounds of the region. Starting from the
top and moving clockwise along the region's border, there are three crossings:

1. Scope to scope – from the module wrapper of the code example out to the
   global scope.
2. Scope to global – from the module wrapper of the fs module to the global scope.
3. From fs' module to its exports. This exports value is retained due to a
   persistent link from the module system's cache, not shown here.

Notably, none of these edges flow into the region. Thus the entire region
may be garbage collected at the discretion of V8. This includes the writable
stream and its "finish" and "open" listeners.

If we had removed the "open" and "finish" listeners on "finish", the
graph would look as follows, instead:

 _____________________________
 / \
 ↓ |
 [[global]] "console" |
 ↑ \_______ |
 ░░░|░░░ \ "log" |
 ░░░░░░ | ░░░░░ Object ----→ Function |
 ░░ [[scope]] ░░░░░░░░░░░░░░░░░░░░░░░ |
 ░░░ | \_____________ "require" ░░ |
 ░░ ⅄ \ ░░ |
 ░░________/|\_________ ↓ ░░ |
 /░"fs" | \ Function ░░ |
 ↓ ░░░ | | ░░ |
 Object ░░ | "read" | "write" ░░ |
 ↑ ░░ ↓ ↓ ░░ |
 | ░░ Readable Writable ░░ |
 | ░░░ ↑ | ░░░ |
 ____________/ ░░ __/ | "_listeners" ░░░ |
/ ░░░░░░░░░░░░░░░░ / ↓ ░░ |
| ░░ __________________/ Object ░░ |
| ░░░░ / ░░ ░░ |
| ░░ | ░░ ░░░░░░░░░░ |
| ░░ | ░░░░░░░░░░░░░░░░░░░░ ░░ |
| ░░ | "self" ░░ |
| ░░ | Function Function ░░░░\____________________________
| ░░ | | \___________░░░░░░░░░░░░░░░░░░░ \
| ░░ | ↓ \ ░░░░░░░░░░ |
| ░░ [[scope]] ←-------- [[scope]] ↓ "path" ░░ |
| ░░ | [[scope]] ----→ String("/") ░░ |
| ░░ ⅄ / \____ ░░ |
| ░░ / \_________________________________________ ___/ \ "options" ░░ |
| ░░ | \ / ↓ ░░ |
| ░░ ⅄ Y undefined ░░ |
| ░░ / \__________________________ | ░░ |
| ░░ | \ \ ↓ ░░ |
| ░░ ↓ "data" ↓ "encoding" ↓ "cb" [[scope]] ____________________________/
| ░░??? ??? ??? / Y ░░
| ░░ ___________/ | ░░░░░
 \_____________________________ / "module" ↓ ░░░░░░░
 ░░"exports" \ ↓ variables of fs.js ░░░░░░
 ░░ Object ░░░░░
 ░░ ░░░░░░
 ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

Although it is divided into two subregions, the overall region is the same.
Because the region is the same, the result will be the same – all objects
within that area will be garbage collected. The only way to cause the "open"
and "finish" listeners to leak is to create a leak with relation to the
Writable object. Removing "open" and "finish" on stream completion in such a
program won't prevent the leak, they'll only slow it down. In so doing, they're
more likely to obscure an existing problem than remedy it.

[Fishrock123]

Sounds like this isn't an issue. Reopen if it is.