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

RqMultipart.Base.make() leaves temporary files until the VM terminates #254

Closed
dmzaytsev opened this issue May 6, 2015 · 43 comments
Closed

Comments

@dmzaytsev
Copy link
Contributor

RqMultipart.Base.make() uses deleteOnExit() option for temporary files. It means such files be deleted when the virtual machine terminates. We will get a large number of temporary files with heavy use applications.
I believe we need to use another method for deletion these temporary files. Since temp files in this method uses as buffer for a large requests we could try collect names and force to delete the files every some time interval


- ~~`254-d68bc52e`/#575~~ (by Armin Braun) - `254-40a8e0fe`/#576 (by Nicolas FILOTTO)
@yegor256
Copy link
Owner

yegor256 commented May 6, 2015

@dmzaytsev totally agree, thanks for catching this

@yegor256 yegor256 added the bug label May 6, 2015
@davvd
Copy link

davvd commented May 6, 2015

@dmzaytsev I will find a developer for the task soon...

@davvd
Copy link

davvd commented May 6, 2015

@dmzaytsev there is no milestone yet, so I set it to 1.0

@davvd davvd added this to the 1.0 milestone May 6, 2015
@davvd
Copy link

davvd commented May 9, 2015

@louzar please go ahead, it's your task now, keep this in mind, and don't hesitate to ask any technical questions you may have

This task's budget is 30 mins. This is exactly how much will be paid when the problem explained above is solved. See this for more information

@davvd
Copy link

davvd commented May 9, 2015

@dmzaytsev thanks for the ticket, your account was topped for 15 mins, payment 56894648

@davvd
Copy link

davvd commented May 18, 2015

@Bertram25 this ticket is yours now, please proceed, and keep in mind this. Any technical questions you should ask right here... Total fixed cost of this task is 30 mins (see this for more info)

@davvd davvd added @bertram25 and removed bug labels May 18, 2015
@Bertram25
Copy link
Contributor

@dmzaytsev Hi, :)

What kind of method would be the most suitable to you?

Proposal: Collect the files in an iterable and add a method to make those cleanable before the VM stops (as you requested), leave the deleteOnExit(); statement in place, and add a finalize() method cleaning the file handles, so that we are sure the temp files are either deleted when the gc triggers the parent request object deletion or at least (and still) when the VM stops.

Quick-made Example showing the part permitting to delete the files when the request is deleted by the gc (as said above, we'd still keep deleteOnExit() call to make sure the files get deleted at least when the VM stops.):

// In RqMultipart.Base class
// ...
    // file handler collection
    private final Iterable<File> files;
    private void cleanTempFiles() {
        for(File file : files) {
            // TODO: Check for file existence before deleting it
            // if we're going to keep deleteOnExit() in make()
            // add try catch
            file.delete();
         }
    }

    // Handles the deletion of files when the object is cleaned up.
    @Override
    protected void finalize() throws Throwable
    {
        try {
            cleanTempFiles();
        } catch (Throwable t) {
            throw t;
        } finally {
            super.finalize();
        }
    }

@dmzaytsev
Copy link
Contributor Author

@Bertram25 hi,
According to the Javadoc

Called by the garbage collector on an object when garbage collection determines that there are no more references to the object.

We don't know when the garbage collector will call finalize() It may happen that we will have a large number of temporary files until the garbage collector will do its job

@yegor256
Copy link
Owner

@dmzaytsev @Bertram25 I think the best place to delete the file is in close() method of InputStream we return at body() of our "parts". Here is the scenario:

Request req = // .. incoming request
final Request part = new RqMultipart.Base(req).part("file").iterator().next();
final InputStream body = part.body();
final byte[] content = new byte[1_000_000];
body.read(content);
body.close(); // boom! here we delete the file

make sense?

@dmzaytsev
Copy link
Contributor Author

@yegor256 @Bertram25 Let's try, I agree

@Bertram25
Copy link
Contributor

@yegor256 @dmzaytsev Thanks for the hints!
Here is a preliminary commit made for review: Bertram25@b1b92c8

Am I doing it the right way? If so, and you agree to give me one or two more days, I'll finish this commit with proper style and an unit test.

Best regards,

@dmzaytsev
Copy link
Contributor Author

@Bertram25 Why do we need TempReadableByteChannel?
let's implement Yegor's suggestion

I think you should use a decorator for InputStream here
https://github.com/yegor256/takes/blob/master/src/main/java/org/takes/rq/RqMultipart.java#L253

this decorator have to use CapInputStream and File in its constructor
and override close() to close the file as written in the Yegor's comment above

@yegor256 correct me if I wrong

@yegor256
Copy link
Owner

@Bertram25 I'm with @dmzaytsev on this

Bertram25 pushed a commit to Bertram25/takes that referenced this issue May 29, 2015
For yegor256#254

I thus added a decorator to InputStream to permit deletion of the temp file
when calling close() as advised by @dmzaytsev

Thanks to @yegor256 and @dmzaytsev for the hints.
Bertram25 pushed a commit to Bertram25/takes that referenced this issue May 29, 2015
…dies are closed.

For yegor256#254

I thus added a decorator to InputStream to permit deletion of the temp file
when calling close() as advised by @dmzaytsev

Thanks to @yegor256 and @dmzaytsev for the hints.
@Bertram25
Copy link
Contributor

@yegor256 @dmzaytsev Indeed! Please accept my apologize for my own lack of sleep and coffee. :)
The PR #318 should now be ready for review.

Thanks for all the advice.

@Bertram25
Copy link
Contributor

@davvd Hi, As said above, the PR #318 is now ready for review.

Best regards,

@davvd
Copy link

davvd commented Jan 19, 2016

@davvd please, assign someone else here

@yegor256 right, I will find someone else, no problem

@davvd davvd removed the @bertram25 label Jan 19, 2016
@exper0
Copy link
Contributor

exper0 commented Jan 19, 2016

@yegor256 @davvd I can finish this

@davvd
Copy link

davvd commented Jan 19, 2016

@alexey-krylov can you please help? Keep in mind this. If you have any technical questions, don't hesitate to ask right here... The budget of this task is 30 mins. This is exactly how much time will be compensated, when the task is completed. Read about our Definition of Done

@alexey-krylov
Copy link

@davvd sorry, i think i have not enough knowledge of 'takes' to fix that in this time. Find someone else please.

@exper0
Copy link
Contributor

exper0 commented Jan 21, 2016

@davvd let me finish it.

@davvd
Copy link

davvd commented Jan 21, 2016

@davvd sorry, i think i have not enough knowledge of 'takes' to fix that in this time. Find someone else please.

@alexey-krylov deducted 30 from your rating :(

@davvd
Copy link

davvd commented Jan 21, 2016

@davvd sorry, i think i have not enough knowledge of 'takes' to fix that in this time. Find someone else please.

@alexey-krylov all right, we'll find someone else for this task

@davvd
Copy link

davvd commented Jan 21, 2016

@davvd let me finish it.

@exper0 you should ask @yegor256 to assign you, he is the architect (I assign developers randomly, always)

@davvd
Copy link

davvd commented Jan 21, 2016

@exper0 the task is yours please proceed

@exper0
Copy link
Contributor

exper0 commented Feb 4, 2016

@dmzaytsev PR #536 merged and closed. Please close request

@dmzaytsev
Copy link
Contributor Author

@exper0 thanks!

@dmzaytsev
Copy link
Contributor Author

@yegor256 could you make a new release ? thanks

@exper0
Copy link
Contributor

exper0 commented Feb 4, 2016

@dmzaytsev just in case - in scope of this task the problem with temporary files / file descriptors is not fully solved. There are a couple of puzzles created in scope of this task. Briefly - in order to clean the file and close actual file descriptor close should be called on request body stream. However, the code is designed in such a way that it's quite difficult to say where is the right place for close.

@dmzaytsev
Copy link
Contributor Author

@exper0 thanks, I hadn't saw the puzzles
@yegor256 we will wait until the puzzles will be completed

@davvd
Copy link

davvd commented Feb 5, 2016

@dmzaytsev 2 puzzles were created here: 254-d68bc52e, 254-40a8e0fe

@davvd
Copy link

davvd commented Feb 6, 2016

@ypshenychka please, let us know what do you think about this ticket, according to our QA rules

@ypshenychka
Copy link

@davvd Quality is good.

@davvd
Copy link

davvd commented Feb 6, 2016

@davvd Quality is good.

@ypshenychka thanks a lot ;)

@davvd
Copy link

davvd commented Feb 6, 2016

@exper0 added 10 mins to @ypshenychka, for QA review, payment code is 76615934... Thank you, I have added 30 mins to your account in payment/transaction "AP-2LK10084ED737235F", time consumed: 348 hours and 30 mins... +30 added to your rating, current score is: +603

@davvd
Copy link

davvd commented Apr 22, 2016

@dmzaytsev the last puzzle 254-40a8e0fe/#576 solved

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

No branches or pull requests

7 participants