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

Temporary files are not cleaned up on app shutdown #4707

Closed
mkurz opened this Issue Jun 18, 2015 · 13 comments

Comments

Projects
None yet
5 participants
@mkurz
Copy link
Member

mkurz commented Jun 18, 2015

If you upload a file via a "multipart/form-data" form and don't consume (e.g. by moving it) or delete the uploaded file within the action method, the temporary files does not get deleted after the action method finishes. Instead the deletion of the file is left to the garbage collector. Also see #3913 for more explanation.
I can confirm this is working.

However when shuting down the app (e.g. via CTRL+C or kill in prod mode) temporary files still remain in my /tmp folder, they don't get deleted.
I consider this a bug.

@richdougherty

This comment has been minimized.

Copy link
Member

richdougherty commented Jun 18, 2015

Me too. Maybe we could put all our files in a temporary directory which gets cleaned up by a shutdown hook.

On a related note, I don't really like the way the delete happens in a finalize method. I'd like us to have a better way of cleaning up resources when a request finishes. Maybe each request could have an object where we could add "request finished" hooks.

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 18, 2015

@richdougherty Just be aware that a temporary file still may be used even after a request is finished, see @jroper's comment about this concern.

@richdougherty

This comment has been minimized.

Copy link
Member

richdougherty commented Jun 18, 2015

Yes I'd seen that discussion. I'd rather the user explicitly does something if they want to keep it around after the request is complete, e.g. some sort of acquire/release operation, or maybe moving the file elsewhere. But I've taken us on a tangent; this ticket is just about cleaning up when the application shuts down.

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

@richdougherty Can't we simply use the deleteOnExit() method? See here.

Requests that the file or directory denoted by this abstract pathname be deleted when the virtual machine terminates.

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

deleteOnExit() seems a bad idea as it leaks memory. Googled around a bit and it seems we should avoid it...

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

I opened PR #4762 as a proposal to fix this issue. Let me know what you think.

@marcospereira

This comment has been minimized.

Copy link
Member

marcospereira commented Jun 25, 2015

My two cents:

This should be configurable since there are no guarantees that files are safe to be deleted during an application restart. So we can have a configuration that enables to following behaviors:

  1. If configured to do automatic cleanup of temporary files, we can do this via deleteOnExit or after request was fully processed. By the way, @mkurz the bug you linked about leaks on deleteOnExit is marked as resolved. Is this still an issue?
  2. If configured to not do automatic cleanup of temporary files, users will have to handle when files are delete by themselves. We can help them with that making TemporaryFile extends java.lang.AutoCloseable (and Scala equivalent) and instruct users to use try-with-resources statement.

This way we can cover both users that do asynchronous processing of files and also users doing simple uploads and keeping files directly at the file system.

What do you think?

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

@marcospereira

...the bug you linked about leaks on deleteOnExit is marked as resolved. Is this still an issue?

Yes, the bug is still an issue. They closed the bug because it actually was about ImageIO using deleteOnExit. If you read through the report you will find that they just avoid deleteOnExit now:

This bug, as described in the description section and relating the Image IO code that uses deleteOnExit to ensure that temporary files are cleaned up, has been fixed by 6291034. It now avoids deleteOnExit completely and uses its own shutdown hook to remove these files. So I am closing this bug as a duplicate of it.

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

If you see my PR #4762, the clean up at shutdown (=deletion of the folder) is just a fall back in case the garbage collector didn't run yet and didn't clean the files already. In the ideal case everything was cleaned up before and the folder would be empty anyway.

(I think) you are talking about the same as @richdougherty mentioned, I think we should open another issue for this because it's about a new way of handling temporary files, but my PR was just intented to fix a current bug. (Right now it isn't expected, not at all guaranteed and also not intended that temporary files survive an app restart).

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jun 25, 2015

(Also, personally I would prefer not to break the handling of temporary files between Play 2.4 and Play 2.5. I would wait for 3.0 for such a change.)

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jul 1, 2015

Fixed via #4762

@mkurz mkurz closed this Jul 1, 2015

@jroper jroper added this to the 2.4.2 milestone Jul 3, 2015

@phleemac

This comment has been minimized.

Copy link
Contributor

phleemac commented Jul 29, 2015

mkurz, could you please also add the new DefaultTemporaryFileCreator to the injector in play.api.BuiltInComponents? Got InstantiationException on receiving multipart/form-data in my project which use Macwire for compile-time dependency injections. Should I open a new issue for this?

@marcospereira

This comment has been minimized.

Copy link
Member

marcospereira commented Jul 29, 2015

@phleemac please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.