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

Fix local base image temp directory cleanup #2018

Merged
merged 5 commits into from
Sep 24, 2019

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Sep 23, 2019

Fixes #2016. Rather than using a shutdown hook to clean up temporary directories, we keep track of them in a thread-safe set that gets passed into the build steps and iterate through them when the build steps finish. I'm not sure how I feel about passing around a list that gets modified inside the build steps, but I think it's overall simpler than doing something like returning multiple values (the temp directories + whatever the steps were originally returning) and collecting them in StepsRunner.

Also, right now the only two steps that use temporary directories are SaveDockerStep and ExtractTarStep, which happen one after the other, so the thread-safe data structure may be unnecessary at the moment.

@TadCordle TadCordle requested a review from a team September 23, 2019 20:54
@TadCordle
Copy link
Contributor Author

I added TempDirectoryProvider, which had a lot in common with TemporaryDirectory, so I ended up just removing the latter completely. I think this is nicer.

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice!

@TadCordle TadCordle merged commit 8d6efa5 into master Sep 24, 2019
@TadCordle TadCordle deleted the i2016-fix-temp-dir-cleanup branch September 24, 2019 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileOperations.deleteRecursiveOnExit() doesn't seem to work
4 participants