ID Lock Code Review

[hard basin]

Hello!

I'm working on a program where I have the following running:

• User requests a container to be provisioned.
• Host machine ensures that that container doesn't already exist.
• Host machine starts the container.
• Host machine adds the container to its list of containers.

Of course, if a user decides to spam requests, I could end up with a race condition where multiple containers are created. I want to fix that by putting a lock over the entire initialization method, but I want it to be able to run concurrently with other initialization requests, so long as they aren't the exact same one. So, I need some sort of ID lock where I can lock an ID for the duration of the function, then unlock it at the end. Here's what I've come up with: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b46b9e2883bc5f08709c519b021afc7e.

This is the sort of territory where I'm liable to create deadlocks (or locks that simply don't do anything), so I'm wondering if somebody could:
A. Take a look over it and tell me what I'm doing wrong.
B. Point me towards a library that implements this exact thing which has somehow eluded me.

Thank you!

[lavish verge]

FWIW, it sounds to me like you're complicating things by making the lock exist only in this "held lock list"

Presumably you have somewhere some data structure that keeps track of which containers currently exist

You can have that data structure own a mutex, right next to where you keep other info about the current state of the container

This avoids the complications of

• if let Some(prev_lock) = prev_lock
• HeldLockGuard
  because you're not trying to manage a thing where the locks are dropped when not in use

(This means your data structure for what containers currently exist should also include ones that are being initialized, not just ones that are ready to use.)

[hard basin]

Yeah, really the main hurdle is making that data structure support initializing containers, since I have to modify every other piece of code that does something with it. Not impossible, obviously, but it means everything else has to bear the consequences of this. The other issue is I have to remove it on initialization failure, which means I still have to have a weird system like this or else I can end up with:

• T1: Init
• T1: Create lock
• T2: Init
• T2: Wait Lock
• T1: Init Failure
• T1: Remove Lock
• T3: Init
• T3: Create Lock
• T3: Init Container
• T2: Init Controller

[lavish verge]

Not impossible, obviously, but it means everything else has to bear the consequences of this.
anything that wants to use an initialized container can potentially benefit from knowing that the particular ID is in the state "just a sec and there will be one"

The other issue is I have to remove it on initialization failure
You can express that as a cleanup operation like

• iterate over table
• try_lock() on the entry. if this fails, do nothing
• if it is in the state "failed to initialize", and the Arc to it has exactly 1 strong reference (the one you're borrowing from the table),
• then delete that entry from the table
  You avoid deadlock and other concurrency problems by saying it's okay to not successfully cleanup everything — it's a sort of garbage collector, rather than an enforced invariant

[hard basin]

True, but that likely won't be very many (if any) containers. The code that reads from the list mostly has to do with pruning containers and reporting which ones are currently active (which should happen after initialization anyway, but I might not have implemented it yet). Neither of these really benefit from waiting for a container to be initiated.

[lavish verge]

I don't mean that other things should wait, I mean that there is probably benefit to having the container listed as present even when it is not yet initialized

you're going to have a harder time getting this right if you try to avoid keeping the state-machine of each container in a single location

[hard basin]

try_lock is a good idea. At first I was thinking of using the strong count, but then I realized it shouldn't be needed if everything is written such that the lock is always safe to remove from the list since that means we dropped before another thread tried to lock. I might take the garbage collector approach though, since it seems easier to reason about.

Hmm, I'll have to think about it. There is one part that would be simplified if I kept track of initializing containers, but everything else will have to be changed to work with it.

[devout moss]

To elaborate on this a bit, concurrent memory reclamation is a notoriously nightmarish problem

The usual solution is GC.

This is also true in languages that don't place a GC on all values: the arc_swap crate comes with a mini-GC, for example, and anything using "hazard pointers" is basically using a GC

[lavish verge]

I mean GC only in the sense of "eventually rather than synchronously drops things"