Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Refactor NewSourceManager() to give more control over lockfile interaction #868

Closed
sdboyer opened this issue Jul 21, 2017 · 2 comments
Closed

Comments

@sdboyer
Copy link
Member

sdboyer commented Jul 21, 2017

#853 introduced more sophisticated handling of the sm.lock lockfile created by NewSourceManager() to guarantee only one process is working on the cache directory at a time. This is a nice addition, as it should solve the easy cases where a lock file gets left around when a dep process quits in a way that prevents cleanup (e.g. a panic). That's a fairly rare case right now, but still - a benefit.

However, #853 also changes the logic so that, rather than erroring out immediately if the lockfile exists (and refers to a valid process), it blocks. This is primarily a crude mechanism of allowing e.g. a CI server to run multiple jobs simultaneously - even though they can't actually proceed in parallel, they at least won't quit if there's another process working in the cache; they'll just wait for
the other process to end, then proceed. (This crude mechanism could conceivably create livelocks in systems relying too heavily on it, but that's a different problem).

The main issue I want to deal with here is expanding the API to give the caller of NewSourceManager() more granular control over this process. I don't know exactly what the API should look like, but we can't just change this call such that it can block for arbitrarily long, without providing more control mechanisms to the caller.

If we can avoid it, I would probably prefer that we NOT use context.Context for this specific purpose. NewSourceManager() also needs to take a context, but that's more about allowing termination of the *sourceMgr once it's created and running - not controlling these initial setup semantics. Apart from that, though, I'm pretty open to ideas; we don't have to worry about preserving the existing signatures.

@sdboyer
Copy link
Member Author

sdboyer commented Jul 21, 2017

cc @matjam

@sdboyer
Copy link
Member Author

sdboyer commented Oct 2, 2017

seems like this actually isn't the route we necessarily want to go. can reopen later if we change our minds.

@sdboyer sdboyer closed this as completed Oct 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant