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

Uncaught exception #8

Closed
satazor opened this issue Nov 20, 2012 · 11 comments
Closed

Uncaught exception #8

satazor opened this issue Nov 20, 2012 · 11 comments

Comments

@satazor
Copy link

satazor commented Nov 20, 2012

I noticed that node-tmp is not cleaning the temporary directories if node dies on an uncaught exception.
This could be fixed with:

process.on('uncaughtException', function (err) {
  // Cleanup here
  throw err;
});

Any reasons for not doing so?

@raszi
Copy link
Owner

raszi commented Nov 21, 2012

I'm not sure it's a good idea.

If there is an uncaught exception then something went really wrong, and one would check the reason of it. And it's better to keep the state as it was (and this state also includes the temporary files).

Of course you could argue with this and state your point.

@satazor
Copy link
Author

satazor commented Nov 21, 2012

While I agree that something is wrong with code, there is bugs in almost every program. By adding this feature, we would accomplish graceful degradation regarding the cleanup.

If you do not implement this, can you at least expose the _garbageCollector function?

@raszi
Copy link
Owner

raszi commented Nov 21, 2012

Yeah, I can absolutely do the latter. And I can even create a method which will do the graceful cleanup if needed.

@satazor
Copy link
Author

satazor commented Nov 21, 2012

That would be great!

@raszi raszi closed this as completed in 16f4e8b Nov 22, 2012
@raszi
Copy link
Owner

raszi commented Nov 22, 2012

I implemented a graceful cleanup option.

Could you take a look please and check that is what you needed?

@satazor
Copy link
Author

satazor commented Nov 22, 2012

Sure, will test it tonight.

@satazor
Copy link
Author

satazor commented Nov 22, 2012

@raszi seems to be working, can you please push a new npm release?

@raszi
Copy link
Owner

raszi commented Nov 22, 2012

of course, I was waiting for your feedback.

@satazor
Copy link
Author

satazor commented Nov 23, 2012

@raszi there is one problem with the implementation atm.

Because _removeObjects.push is only being done when the directory is created, there is situations in which the directory gets created but an exception is thrown before the actual mkdir callback is fired.

So, in cases in which there is an heavy usage of tmp.dir (like twitter's bower), some directories won't get gc.

Would it be possible to push to the _removeObjects array before the actual mkdir? The only thing that needs to be taken care of is that if the mkdir failed, it must be removed from the array.

@raszi
Copy link
Owner

raszi commented Nov 23, 2012

If I would put the temporary filename before the creation to _removeObjects that would cause a race-condition, because before the temporary file or directory creation another file or directory could be created by any other process in the system.

I want to avoid this. So if the price is to leave some file or directory after an uncatched exception, that's fine for me.

@satazor
Copy link
Author

satazor commented Nov 23, 2012

Good point altough likely impossible to happen, specially if a prefix is used. Still, I understand your point

raszi added a commit that referenced this issue May 11, 2015
@silkentrance silkentrance mentioned this issue Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants