zlib.*Sync methods fire asynchorous callbacks

[ChALkeR]

https://github.com/iojs/io.js/blob/v2.0.1/lib/zlib.js#L458 — this line is called even for *Sync methods. That's not the only one.

See #1479 and #1665 (comment)

[ChALkeR]

@trevnorris

[ChALkeR]

@Qard It's not the garbage collector that is broken as you supposed in #1665 (comment), it's zlib.*Sync methods not working as expected.

[ChALkeR]

That's since 07d3b21, I suppose.
I might be wrong, I haven't tested that commit.
@isaacs

[Qard]

I didn't say garbage collection was broken, I said your expectations of it were incorrect and that there needed to be documentation to explain that.

In this case, the objects are not collectable simply because they are still accessible by future code--the close emitting function. That can be fixed.

JavaScript in general is not a good language for sync looping though, as you need to trigger GC manually and hope that enough things are collectable to not rapidly run out of memory.

[trevnorris]

@ChALkeR Can you give me a code example where that manifests itself as a problem?

[ChALkeR]

@trevnorris I initially posted that to the issue: #1665 (comment) has the code sample, #1479 is the original report.

@targos commented 20 days ago
I was running a script to read sequentially a lot of gzipped files, and print on stdout the result of a computation for each file.
After about 16'000 files, the process just stopped and Killed was printed on the terminal.

[trevnorris]

@ChALkeR Thanks. IMO it's a bug.

[ChALkeR]

@indutny On a side note, zlib.* methods (both sync and async) are collected only during full GC runs. Could something be done with that?
Edit: It's Buffers fault: #1671.

[ChALkeR]

@bnoordhuis
But the reason to defer the event is to prevent infinite recursion. I'm not sure if this is fixable without breaking something else.

Is that the case for *Sync methods? Will it be safe not to defer the event for *Sync methods?

[bnoordhuis]

Is that the case for *Sync methods?

Yes. Events always need to be asynchronous. The problem in a nutshell:

// example 1
function next() {
  var t = task();
  t.on('complete', next);
}

// example 2
function next() {
  task(next);
}

In the first example, if 'complete' is synchronous, you don't have time to attach an event listener.

In the second example, if the callback is synchronous, you get infinite recursion.

[ChALkeR]

@bnoordhuis But those events are not public in *Sync case and can't be used outside of the core code. I am not even sure if they are needed in the *Sync case.

[trevnorris]

@bnoordhuis just curious, is there ever a time where a synchronous task allows adding event listeners?

[piscisaureus]

@bnoordhuis just curious, is there ever a time where a synchronous task allows adding event listeners?

It happens in other places too (e.g. fs.createSyncReadStream(), process.stdout).
It pretty much always causes problems, and I think it's a broken pattern that we shouldn't use.

[trevnorris]

@piscisaureus Sorry, I should have clarified the question. Is there ever a time where the public API allows attaching events to a synchronous task?

[bnoordhuis]

I'm not aware of any such APIs.

[trevnorris]

@bnoordhuis Thanks. Just wanted to confirm that the reason this can't be done is because of our own internal way of handling these events.

So, it seems at some point this should be fixed. Not volunteering for it, and it would probably help to have a discussion about it before anyone actually got started.

[Trott]

For the specific problem demonstrated with @ChALkeR's test code:

'use strict';

var zlib = require('zlib');
var data = 'abcdefghijklmnopqrstuvwxyz';
var gzipped = zlib.gzipSync(data);

var time = process.hrtime();
var count = 0, limit = 800000, block = 20000;

function call() {
    var contents = zlib.gunzipSync(gzipped);
    count++;
    if (count % block === 0) {
        var t = process.hrtime(time);
        console.log(count + ': ' + block / (t[0] + t[1] / 1e9) + ' per second ' + JSON.stringify(process.memoryUsage()));
        time = process.hrtime();
    }
}

for (var i = 0; i < limit; i++) {
    call();
}

This appears to be fixed by just getting rid of the mechanism to emit close events altogether on the internal-ish Zlib object. It is not used internally and it is not documented. Still, that would probably be a semver-major move and perhaps an undesirable one? #5708

Another option is to add an options object as a parameter to (again, internal-ish) Zlib.prototype.close() and set an option indicating that the thing is invoked by a *Sync() function (and thus no event should be queued). Effective but perhaps too inelegant? #5707

[Trott]

I believe #5707 is a fix for this. PTAL