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

Asynchronous Cleanup Callback #155

Closed
8 tasks
freitagbr opened this issue Nov 17, 2017 · 6 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Closed
8 tasks

Asynchronous Cleanup Callback #155

freitagbr opened this issue Nov 17, 2017 · 6 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Assignees

Comments

@freitagbr
Copy link

Operating System

  • Linux
  • Windows 7
  • Windows 10
  • MacOS
  • other: all

NodeJS Version

  • 0.x
  • 4.x
  • 6.x
  • 7.x
  • other: all

Tmp Version

v0.0.33

Expected Behavior

The cleanup callback provided by tmp.dir and tmp.dirSync removes the temp directory asynchronously.

Experienced Behavior

The cleanup callback provided by tmp.dir and tmp.dirSync removes the temp directory synchronously. It would be awesome to have an option to make the cleanup callback asynchronous, so that removing directories with a lot of contents doesn't block.

@silkentrance
Copy link
Collaborator

silkentrance commented Nov 29, 2017

@freitagbr would you require a callback for notification on when tmp is done removing the temporary objects?

Oh, wait, there is already the next callback that can be passed to the cleanupCallback. So you are free to use that if you like, as it is optional.

@silkentrance
Copy link
Collaborator

@freitagbr the problem, though, is that even on process.nextTick, the operation will still be blocking.

How do you envision tmp to be removing such large directories? And is it even an issue, considering that tmp will be removing these files on process exit only or whenever the cleanup callback is being called.

What is your actual use case here? Why do you keep that many temporary files in a single location so that it will stall the operation of your application?

@freitagbr
Copy link
Author

My use case is that I'm creating temp directories as a sandbox to unzip .zip files (potentially very large, say 100GB+), manipulate the contained files, repackage them, and then send them off somewhere else.

Sure, if the cleanup callback is deferred with process.nextTick, that will not be an improvement, because fs.unlinkSync is still synchronous. My idea is to implement an asynchronous cleanup function that uses fs.unlink and fs.rmdir, so that cleaning up a large amount of data does not block. rimraf has a thorough implementation as an example.

@silkentrance
Copy link
Collaborator

silkentrance commented Dec 1, 2017

This is even more involved as we also install a process.exit listener to handle graceful cleanups whenever a process exists or gets terminated.

Inside these handlers we cannot run any asynchronous code. Yet another problem to be solved, yeah.

@freitagbr
Copy link
Author

I see that _prepareTmpDirRemoveCallback adds the cleanup callback to _removeObjects (which are cleaned up in the process.exit handler), and returns the cleanup callback to the user. Perhaps, in an async mode, an async cleanup callback is returned to the user, but the sync cleanup callback is still added to _removeObjects. This handles a process.exit occurring in the middle of the cleanup, allowing the remnants to be removed synchronously. The async cleanup callback would have to remove the sync callback from _removeObjects at the end of its operation.

silkentrance added a commit that referenced this issue Dec 1, 2017
fix existing async tests, finally
silkentrance added a commit that referenced this issue Dec 1, 2017
fix existing async tests, finally
@silkentrance silkentrance mentioned this issue Dec 1, 2017
@silkentrance silkentrance self-assigned this Dec 1, 2017
This was referenced Dec 1, 2017
@silkentrance
Copy link
Collaborator

silkentrance commented Dec 1, 2017

@freitagbr thank you very much for your invaluable input. in #161 I do exactly what you proposed and it works like a charm. However, we still need to reduce the extra amount of code and by that I mean we have to just eliminate it by using rimraf. Not for files, though, as these still need to be closed in case of an open file descriptor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants