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

Add a unique integer to the sandbox dir of mill --no-server #3396

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

lefou
Copy link
Member

@lefou lefou commented Aug 19, 2024

Fix #3395

@lihaoyi
Copy link
Member

lihaoyi commented Aug 19, 2024

If we're going to create a new folder for every run, we should do a best effort to clean it up at the end in a finally block. Won't be able to catch every scenario, but at least in the happy path we won't leak folders on disk

@lefou
Copy link
Member Author

lefou commented Aug 19, 2024

Yeah, makes sense. I can add it. Should we prepare for the scenario where we want to keep the sandbox dir, e.g. for later inspection in case of a non-zero exit code?

@lihaoyi
Copy link
Member

lihaoyi commented Aug 19, 2024

How about we clean up asynchronously? e.g.

  1. Each process takes a lock on the sandbox,
  2. Every subsequent process lists the existing sandboxes and probe the lock
  3. If the lock is available, it means the process has exited, and we can delete the folder

That way for the common workflow of running Mill from the command line, we don't delete stuff immediately, but only upon the next command.

Probably something similar can be done for the long lived mill-worker-* folders as well: if the folder exists but both the clientLock and processLock are available, it means the process has exited and can be cleaned up

@lefou
Copy link
Member Author

lefou commented Aug 19, 2024

Might work. But is a bit more work than I anticipated with this quick fix. Feel free to take over this PR or merge right away and open another one for the probe-based approach.

@lihaoyi
Copy link
Member

lihaoyi commented Aug 19, 2024

Let's merge it as is then

@lefou
Copy link
Member Author

lefou commented Aug 20, 2024

@lihaoyi I added basic cleanup and opened a new issue for the asynchronous cleanup idea.

@lefou lefou merged commit 3c42868 into main Aug 21, 2024
32 checks passed
@lefou lefou deleted the sandbox-no-server-dir branch August 21, 2024 12:12
@lefou lefou added this to the 0.12.0 milestone Aug 21, 2024
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

Successfully merging this pull request may close these issues.

Don't share working dir for parallel running --no-server instances
2 participants