Unzip stream closed event not firing until focus change

[kevinsawicki]

Steps to reproduce:

1. Install the unzip module.
2. Open the dev tools
3. Paste the following code into the console, replacing the path with one to any local .zip file:

require('fs').createReadStream('/a/file.zip')
             .pipe(require('unzip').Parse())
             .on('close', function(){console.log('stream closed')})

1. Hit enter
2. stream closed will not be logged
3. Click the editor window
4. stream closed will be logged

[zcbenz]

I am sure that the underlying native code works fine, since piping the file stream to process.stdout works as expected, so the bug must hides in the unzip module's code, I guess it's related to how process.nextTick is implemented in atom-shell.

It would take me some time to find it out since unzip has used many third party modules.

[zcbenz]

This bug is again caused by the setImmediate function.

The unzip module uses setImmediate in Parse.prototype._flush to wait for more data when the file was read but the data was not completely parsed. This trick relies on this behavior: the setImmediate should run the callback after other uv operations, however since the setImmediate is implemented via WebKit's setTimeout, the callback passed would be scheduled to run in WebKit's message loop, which is independent with uv loop, so it happens that the callback passed in setImmediate just keeps running but the other operations pending in uv loop just do not get a chance to run, until something accidentally activates the uv loop.

Implementing the setImmediate with node's timers module would fix this.

The process.nextTick implementation in atom-shel does have the same problem with setImmediate, and I doubt atom/atom@70e414b is caused by this. I'll switch both functions to use their original implementations.

[zcbenz]

Fixed with 25d9c1a and electron/node@4b4c40a.

Instead of using node's original implementation of setImmediate, I just make sure when setImmediate is called the uv loop would also be activated (or interrupted), so it can have the behavior expected by unzip module. Using node's setImmediate would cause interleaves of two message loops and two V8 contexts, which is the thing I want to avoid most.

[kevinsawicki]

👍 awesome, thanks for the quick fix!

[nathansobo]

Is this something that Ben Noordhuis' changes to node to support multiple
contexts would fix?

On Tue, Jul 9, 2013 at 9:49 AM, Kevin Sawicki notifications@github.comwrote:

[image: 👍] awesome, thanks for the quick fix!

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-20683820
.

[zcbenz]

@nathansobo no, this is caused by our message loop integration, we have to use this solution unless node supports multiple message loops (which I don't think node would ever do, they gave up once when trying to adding isolates to node).

[kevinsawicki]

Has this been released?

[zcbenz]

Yes it has been released, but I found it only fixed zip files of smaller sizes, there must have been some other bugs.

[zcbenz]

Fixed with electron/node@d0ccec3.

Now atom-shell just uses node's implementation of setImmediate, otherwise it's quite hard to make unzip work as expected.