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 not cleaned up after Maven plugin execution #22112

Closed
wants to merge 1 commit into from

Conversation

dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Jun 25, 2020

Hi,

this PR fixes #22108 by calling deleteOnExit on the temporary file and additionally prevents creating the file all together if we don't exceed the thresholds for which it was introduced.

Since deleteOnExit is only executed on "normal" VM termination, there is still a slight chance that temporary files will not be cleaned up when the VM dies for unknown reasons. But with the additional lazy init they probably won't be created in the first place for most apps.

Let me know what you think.

Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 25, 2020
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 26, 2020
@wilkinsona wilkinsona added this to the 2.3.x milestone Jun 26, 2020
@snicoll snicoll assigned snicoll and unassigned snicoll Jul 7, 2020
@wilkinsona wilkinsona changed the title Cleanup temporary files after Maven plugin execution Temporary files not cleaned up after Maven plugin execution Jul 9, 2020
@wilkinsona wilkinsona self-assigned this Jul 9, 2020
@wilkinsona
Copy link
Member

Thanks for the PR, @dreis2211. Ideally, I'd like a solution that can be tested but that's not easy to do with deleteOnExit(). I made an attempt that makes EntryWriter Closeable but the knock-on effect of that was broader than I'd like. Having spent some time in the code trying to find a nice solution, I've found myself wondering why we're using a file at all. It's inconsistent with how we handle calculating the hash for unpack comments which is currently done entirely in memory. I'm going to put this one on hold for now while we take a step back and consider things a bit more broadly.

@wilkinsona wilkinsona removed this from the 2.3.x milestone Jul 9, 2020
@wilkinsona wilkinsona added status: on-hold We can't start working on this issue yet status: waiting-for-triage An issue we've not yet triaged and removed type: bug A general bug status: waiting-for-triage An issue we've not yet triaged labels Jul 9, 2020
@dreis2211
Copy link
Contributor Author

@wilkinsona I actually share the testability concern, I had this too while developing.
My concern on the other hand is that - depending on how long this will be on-hold - we won't solve the bug for users in the upcoming 2.3.2 release. What about merging this for now to improve things for users, while opening a follow-up issue that tries to improve things on a broader level?

@wilkinsona
Copy link
Member

It would certainly be nice to get a fix in some form or other into 2.3.2. I've labelled #22108 for team attention so we'll hopefully discuss it later today. Let's see what comes out of that and then make a decision on the best way to proceed.

@dreis2211
Copy link
Contributor Author

Sounds reasonable. Thanks

@philwebb
Copy link
Member

We discussed this today and decided that we should merge this in 2.3.x and also bump up the threshold to reduce the number of files written. We'll perhaps try to look at refactoring that EntryWriter code sometime in the future to make them close aware.

@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge type: bug A general bug and removed status: on-hold We can't start working on this issue yet labels Jul 10, 2020
@philwebb philwebb added this to the 2.3.x milestone Jul 10, 2020
@wilkinsona wilkinsona removed the for: merge-with-amendments Needs some changes when we merge label Jul 14, 2020
@wilkinsona wilkinsona modified the milestones: 2.3.x, 2.3.2 Jul 14, 2020
@wilkinsona
Copy link
Member

Thanks again, @dreis2211!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants