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

mutex.lock should give you a disposable token, rather than the mutex itself being disposable #27

Closed
bakkot opened this issue Jul 29, 2024 · 32 comments

Comments

@bakkot
Copy link

bakkot commented Jul 29, 2024

The readme mentions a possible integration with explicit resource management. It suggests adding mutex.lock and mutex.unlock and making mutexes disposable. I think the latter two points are wrong, and the correct API would look like this:

using void = mutex.lock();
// alternatively, without `using`
let token = mutex.lock();
// ...
token.unlock();

i.e., mutex.lock returns an object with a Symbol.dispose method and a string-named unlock method (as an alias of dispose), rather than the mutex being disposable. This makes it impossible to accidentally unlock the mutex without first locking it.

The tokens need not be shared objects (though I guess they could be, if that's easy to do). Edit: Actually, on talking this over, probably the ideal is for the tokens to be cloneable, in the sense of having explicit support in structuredClone, but not be shared objects.

cc @michaelficarra @lucacasonato

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 29, 2024

Neither are the design we've been working on, though it is close. The approach we've been considering is somewhat similar to the C++ unique_lock, given the flexibility that a UniqueLock provides. This is the version I used in TypeScript's experimentation with shared structs: https://github.com/microsoft/TypeScript/blob/shared-struct-test/src/compiler/threading/uniqueLock.ts

The API I've been discussing with the V8 teams at Google and Microsoft looks something like this:

type Lockable = Mutex; // `| SharedMutex` or others in the future

class UniqueLock<T extends Lockable> {
  constructor(mutex?: Lockable, t: "lock" | "defer-lock" | "try-to-lock" | "adopt-lock" = "lock");
  get mutex(): Lockable | undefined;
  get ownsLock(): boolean;
  tryLock(): boolean; // try to lock the mutex. non-blocking
  lock(): void; // lock the mutex (possibly with a timeout?). blocking
  lockAsync(): Promise<void>; // locks the mutex asynchronously. non-blocking
  unlock(): void; // unlocks the mutex
  release(): void; // detach the mutex without unlocking
  swap(other: UniqueLock<T>): void; // swap state with another mutex
  move(): UniqueLock<T>; // create a new lock with this lock's state, releasing this lock
  [Symbol.dispose](): void; // unlock mutex and release the lock if it is taken.
}

Examples:

// take lock synchronously (off UI thread)
{
  using lck = new UniqueLock(mut);
  
} // lock released

// take lock asynchronously (on UI thread)
{
  using lck = new UniqueLock(mut, "defer-lock");
  await lck.lockAsync();
}

// try to take lock (on or off UI thread) 1/2
{
  using lck = new UniqueLock(mut, "try-to-lock");
  if (lck.ownsLock) {
    ...
  }
}

// try to take lock (on or off UI thread) 2/2
{
  using lck = new UniqueLock(mut, "defer-lock");
  if (lck.tryLock()) {
    ...
  }
}

// take, release, and retake lock (off UI thread)
{
  using lck = new UniqueLock(mut);
  // has lock
  lck.unlock();
  // does not have lock
  lck.lock();
  // has lock again
}

// adopt an existing lock, such as when bootstrapping an application started from another
// short-lived thread that is no longer participating in coordination.
{
  const lck1 = new UniqueLock(mut, "adopt-lock"); // adopts lock that was already taken
  lck1.ownsLock; // true
}

// swapping locks
{
  using lck1 = new UniqueLock();
  {
    using lck2 = new UniqueLock(mut);
    ...
    lck1.ownsLock; // false
    lck1.mutex; // undefined

    lck2.ownsLock; // true
    lck2.mutex; // [object Mutex]

    lck1.swap(lck2);

    lck1.ownsLock; // true
    lck1.mutex; // [object Mutex]

    lck2.ownsLock; // false
    lck2.mutex; // undefined
  }
}

And if/when we later add a SharedMutex for read/write locks, you use UniqueLock for readers and a SharedLock for writers.

By having UniqueLock be a non-shared, local JS object it cannot leak into other threads, while Mutex remains a shared object and is possibly otherwise opaque to consumers. In addition, we can introduce other locking mechanisms later, such as a ScopedLock that implements a deadlock prevention algorithm when you need to acquire locks for multiple resources and want to avoid deadlocks.

@rbuckton
Copy link
Collaborator

Unfortunately, a simple mutex.lock() with a dispose is not sufficient for many of the use cases that we actually need locking for. The above design keeps concerns properly separated and provides the flexibility needed to build more complex coordination mechanisms and deadlock prevention algorithms.

@bakkot
Copy link
Author

bakkot commented Jul 30, 2024

Can you give examples of cases for which the mutex.lock() design is not sufficient?

@rbuckton
Copy link
Collaborator

Can you give examples of cases for which the mutex.lock() design is not sufficient?

I gave a number of examples, above. While fairly simplistic, they do illustrate a number of real-world uses of std::unique_lock from C++ that I found compelling while working with the dev trial.

Also, something like ScopedLock can implement deadlock prevention fairly efficiently in terms of UniqueLock. The only reason I'm not using it in the version of ScopedLock I linked to above is because it's using some internal wrappers around Atomics.Mutex as the dev trial did not contain a UniqueLock or other mechanism that was compatible with using.

@bakkot
Copy link
Author

bakkot commented Jul 30, 2024

Sorry, I meant example use cases. Things you're trying to do with the more complicated API, not just how you call it.

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 30, 2024

Of the above API examples, swap and move are probably the least useful. The most useful being lock/lockAsync, tryLock, unlock, release, and [Symbol.dispose] (which combines unlock() and release()) which all have very relevant use cases.

The use cases for lock/lockAsync and [Symbol.dispose] should be fairly obvious and are equally as relevant to UniqueLock and mutex.lock() designs. tryLock() is highly useful for fast and efficient spin-locking, and when coupled with unlock() can be used to implement low-overhead scoped locking across multiple mutexes with deadlock prevention.

Let's imagine, for the moment, that Mutex had an API like this:

interface Mutex {
    lock(): Disposable;
    lockAsync(): Promise<Disposable>;
    tryLock(): Disposable | undefined;
}

To implement a multi-mutex lock with deadlock prevention, like ScopedLock, you might do something like this:

/**
 * @param {Iterable<Mutex>} mutexes
 * @returns {Disposable}
 */
export function lockAll(mutexes) {
    mutexes = Array.from(mutexes);
    let locks = new Array(mutexes.length);
    let remaining = mutexes.length;
    let index = 0;
    let error;
    let hasError = false;
    while (!hasError) {
        try {
            if (remaining === 0) {
                // all locks taken, return a disposable to release them later
                const scope = new DisposableScope();
                for (const lock of locks.slice(index)) {
                    scope.use(lock);
                }
                for (const lock of locks.slice(0, index)) {
                    scope.use(lock);
                }
                return scope;
            }
            if (remaining === mutexes.length) {
                // always wait for the first lock
                locks[index] = mutexes[index].lock(); // NOTE: lock token allocation
            }
            else {
                locks[index] = mutexes[index].tryLock(); // NOTE: lock token allocation
                if (!locks[index]) {
                    // failed to take lock. release each token and start over at the current index
                    let i = (index + mutexes.length - 1) % mutexes.length;
                    while (remaining < mutexes.length) {
                        // always unlock all locks taken, even if one unlock fails for some reason
                        try {
                            locks[i]?.[Symbol.dispose](); // NOTE: lock token release
                            locks[i] = undefined;
                        }
                        catch (e) {
                            error = error ? new SuppressedError(e, error) : e;
                            hasError = true;
                        }
                        i = (index + mutexes.length - 1) % mutexes.length;
                        remaining++;
                    }
                    continue;
                }
                // lock was taken, move to next mutex
                index = (index + 1) % mutexes.length;
                remaining--;
            }
        }
        catch (e) {
            error = error ? new SuppressedError(e, error) : e;
            hasError = true;
        }
    }

    // if we reach this point, an error occurred. unlock all locks taken in reverse order and throw the error
    let i = (index + mutexes.length - 1) % mutexes.length;
    while (remaining < mutexes.length) {
        // always unlock all locks taken, even if one unlock fails for some reason
        try {
            locks[i]?.[Symbol.dispose]();
        }
        catch (e) {
            error = new SuppressedError(e, error);
        }
        i = (index + mutexes.length - 1) % mutexes.length;
        remaining++;
    }

    throw error;
}

This algorithm steps through all of the provided mutexes, locking each one in order. We only use lock for the first mutex, and use tryLock for subsequent mutexes to avoid a deadlock. If we fail to take a subsequent lock, we must rewind all of the taken locks and start over. When we restart, we restart the process starting at the current index since that is the lock that was in contention.

The downside of this approach is that there is overhead for producing and releasing new disposable lock tokens when there is significant contention. Even though these are nursery objects, that means they are cheap, not free. This is unfortunate because in the ideal scenario, all of the locks we take are expected to at least outlive the function call.

Here is the same algorithm utilizing UniqueLock, tryLock, and unlock:

/**
 * @param {Iterable<Mutex>} mutexes
 * @returns {Disposable}
 */
export function lockAll(mutexes) {
    const locks = Array.from(mutexes, mutex => new UniqueLock(mutex, "defer-lock")); // all allocations occur once
    let remaining = locks.length;
    let index = 0;
    let error;
    let hasError = false;
    while (!hasError) {
        try {
            if (remaining === 0) {
                // all locks taken, return a disposable to release them later
                const scope = new DisposableScope();
                for (const lock of locks.slice(index)) {
                    scope.use(lock);
                }
                for (const lock of locks.slice(0, index)) {
                    scope.use(lock);
                }
                return scope;
            }
            if (remaining === locks.length) {
                // always wait for the first lock
                locks[index].lock(); // NOTE: no allocation
            }
            else {
                if (!locks[index].tryLock()) { // NOTE: no allocation
                    // failed to take lock. release each token and start over at the current index
                    let i = (index + locks.length - 1) % locks.length;
                    while (remaining < locks.length) {
                        // always unlock all locks taken, even if one unlock fails for some reason
                        try {
                            locks[i].unlock(); // NOTE: no release
                        }
                        catch (e) {
                            error = error ? new SuppressedError(e, error) : e;
                            hasError = true;
                        }
                        i = (index + locks.length - 1) % locks.length;
                        remaining++;
                    }
                    continue;
                }
                // lock was taken, move to next mutex
                index = (index + 1) % locks.length;
                remaining--;
            }
        }
        catch (e) {
            error = error ? new SuppressedError(e, error) : e;
            hasError = true;
        }
    }

    // if we reach this point, an error occurred. unlock all locks taken in reverse order and throw the error
    let i = (index + locks.length - 1) % locks.length;
    while (remaining < locks.length) {
        // always unlock all locks taken, even if one unlock fails for some reason
        try {
            locks.unlock();
        }
        catch (e) {
            error = new SuppressedError(e, error);
        }
        i = (index + locks.length - 1) % locks.length;
        remaining++;
    }

    throw error;
}

Here, there is no overhead from object allocation and release during high contention. All of the locks that will eventually outlive the function are allocated all at once.

@bakkot
Copy link
Author

bakkot commented Jul 30, 2024

I see. I agree that the object allocations in that specific algorithm are unfortunate, though I would want to hear from an engine that this matters in practice before committing to a more complex design just to avoid these allocations.

In any case, if the goal was just "avoid allocations in the above algorithm", that would be more simply accomplished by making mutex.lock/mutex.tryLock accept an optional released token which it would then re-lock and return, as in

using token = mutex.lock();
//...
token.release();
// ...
mutex.lock(token);

You would then implement the above algorithm by changing your first implementation as follows:

- locks[index] = mutexes[index].lock();
+ locks[index] = mutexes[index].lock(locks[index]);

- locks[index] = mutexes[index].tryLock();
- if (!locks[index]) {
+ let lock = mutexes[index].tryLock(mutexes[index]);
+ if (lock) { mutexes[index] = lock; /* only does anything the first time through */ }
+ else {

- locks[i] = undefined;

That seems much simpler than the UniqueLock API.

@rbuckton
Copy link
Collaborator

I'm not sure I would agree that is simpler. The mechanics of what you're describing add a lot more moving parts and complex interactions to reason over and a fair amount of back and forth. It's certainly not straightforward.

What I am suggesting isn't complex. You have the mutex, which is purely shared state, and the lock, which is the thread-local mechanism to interact with that state. Everything you want to do to interact with the Mutex is on the lock object:

  • What mutex am I associated with?
  • Do I own a lock?
  • Can I try and lock the mutex?
  • Block until I can lock the mutex
  • Unlock the mutex
  • Release the mutex

The UniqueLock is the resource you hold, and initializing the resource as part of using is the closest application of RAII there is.

In addition, we'd likely change Condition.wait to take the lock (the thing that actually has both lock/unlock) rather than the mutex itself, as that is a far clearer separation of concerns.

IMO, UniqueLock is a very clean and consistent API and is extremely flexible.

@bakkot
Copy link
Author

bakkot commented Jul 30, 2024

In your design, locking a mutex is done by creating a separate object (the creation of which may or may not lock the mutex, depending on a four-state enum argument to the constructor) and/or by calling a method on the separate object. Async locking is necessarily two steps because new cannot be async. The separate object may or may not remain associated with that mutex and there are several methods for juggling this.

In my design, locking a mutex is always done by calling a method on the mutex, which creates or updates a token. Async locking looks exactly like sync locking, just calling the async API instead of the sync API and adding an await. The token has no capabilities at all except the ability to unlock while it is the result of the most recent locking.

If there's some reason to want the extra complexity of UniqueLock, I'm happy to have that conversation. But the UniqueLock design is definitely more complex and has more moving parts. I don't think that's arguable.

The design I've proposed incidentally is quite close to the design in Rust. Indeed if you consider using equivalent to dropping an object in Rust, it's nearly identical. I think we should generally prefer taking our cues from Rust rather than C++.

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

Ignoring the UniqueLock wrapper conversation and responding narrowly to the token-based design vs a simple lock/unlock design, IIUC the main advantage is "can't unlock before locking"? If so, I'm not sure I buy that, because once you have the token, you can still unlock it multiple times, so we still have to spec what it means to unlock something that's not locked. Compared to Rust, which I think can leverage its static semantics to prevent you from doing the obvious bad thing. That is, the main advantage of the token design is an easier happy path, not any kind of correctness guarantees, right?

Plus the allocation isn't great. All things considered I'd like to enable the fine-grained locking use case (like, if an app wants a lock per object).

Given that, my current preference is the simpler lock/unlock design (with a simple withLock callback-taking wrapper, perhaps).

@lucacasonato
Copy link
Member

you can still unlock it multiple times

No you can't - every tokens unlock method can only be called once.

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

The main advantage is that it semantically ties the capability to unlock to the corresponding locking. You can't accidentally unlock a mutex held by someone else. That's the correctness guarantee it gives you.

we still have to spec what it means to unlock something that's not locked

There's an important difference here: calling .unlock() on a token which has already been unlocked does nothing, even if the mutex is currently locked. Whereas locking a mutex once and then accidentally calling .unlock() twice can do arbitrarily bad things, because you may be releasing a lock which doesn't belong to you (if someone else has acquired it between the first and second call).

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

No you can't - every tokens unlock method can only be called once.

You can't statically prevent it from being called. You still have to spec what it does a second time if it is called, like throw, or do nothing.

There's an important difference here: calling .unlock() on a token which has already been unlocked does nothing, even if the mutex is currently locked. Whereas locking a mutex once and then accidentally calling .unlock() twice can do arbitrarily bad things, because you're releasing a lock which doesn't belong to you.

Ah true, good point. For blocking locks we can do something simpler, like checking if the current agent holds the lock on the mutex. For async locks the token proposal seems superior.

The allocation per locking operation annoyance stands, but mainly for the sync case. The async case is so expensive already, a small token allocation won't matter.

Perhaps something like async locks hand out tokens, and sync locks/unlocks need to check for the current agent having acquired the lock already?

Edit: For a uniform API, there can also be a single, pre-allocated token for the blocking case per agent.

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

For blocking locks we can do something simpler, like checking if the current agent holds the lock on the mutex. For async locks the token proposal seems superior.

Actually I take this back. That is too inflexible. I see no reason why one agent can't lock a mutex, then another agent unlocks the mutex, if a critical section spans several agents. The token approach doesn't have this issue assuming you can clone the token, nor does the simple lock/unlock API.

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

For blocking locks we can do something simpler, like checking if the current agent holds the lock on the mutex.

We have concurrency within agents, and you can hold a lock across async turns. That means that multiple pieces of code within the same agent might be trying to lock/unlock the same lock, which can lead to the same problem as would happen across threads. Consider:

async function foo() {
  await someSetup();
  try {
    mutex.lock();
    await someOperationWhichNeedsTheLock();
    // done, we can release the lock immediately
    // but we stay in the `try` so we can get our error handling
    mutex.unlock();
    await someOtherOperation();
  } catch (e) {
    handle(e);
  } finally {
    // make sure we release the lock when the function is done
    mutex.unlock();
  }
}

await Promise.all([foo(1), foo(2)]);

This can lead to the following execution:

  • both calls are in the await someSetup
  • foo(1) finishes setup and gets the lock
  • foo(1) gets past the await someOperationWhichNeedsTheLock while foo(2) is still in setup
  • foo(1) releases the lock and enters await someOtherOperation
  • foo(2) finishes setup and gets the lock
  • foo(2) pauses at await someOperationWhichNeedsTheLock
  • foo(1) finishes with someOtherOperation and enters its finally, which causes it to release the lock while foo(2) is still in the section which needs it

So you end up executing code while you think you have the lock, but you don't, even though there's only one agent.

It is much harder to make this mistake with tokens.

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

Updated to avoid the possibility of deadlock, just to make it clear this can happen in otherwise-reasonable code:

let resourceMutex = new Atomics.Mutex;
let deadlockMutex = new Atomics.Mutex;

async function foo() {
  try {
    await deadlockMutex.lockAsync();
    for (let i = 0; i < 1e6; ++i) {
      // use the sync locking API because we rarely need any `await`s in this loop
      // deadlockMutex guarantees only one async function is in this loop at a time, so we can't deadlock
      resourceMutex.lock();
      someSyncOperationWhichNeedsTheLock();
      if (unlikely()) await someAsyncOperationWhichNeedsTheLock();
      resourceMutex.unlock();
      someExpensiveComputation(); // we want to allow other threads to use the resource while this happens
    }
    deadlockMutex.unlock();
    await someOtherOperation();
  } catch (e) {
    handle(e);
  } finally {
    // make sure we release the locks when the function is done even in case of errors
    deadlockMutex.unlock();
    resourceMutex.unlock();
  }
}

await Promise.all([foo(1), foo(2)]);

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

It is much harder to make this mistake with tokens.

Yes, I buy that. Ideas on how to overcome the allocation issue for the sync case?

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

Ideas on how to overcome the allocation issue for the sync case?

I would have mutex.lock accept an optional token which it would re-lock and return. In the code above that would become

let resourceMutex = new Atomics.Mutex;
let deadlockMutex = new Atomics.Mutex;

async function foo() {
  let resourceToken;
  let deadlockToken;
  try {
    deadlockToken = await deadlockMutex.lockAsync();
    for (let i = 0; i < 1e6; ++i) {
      // use the sync locking API because we rarely need any `await`s in this loop
      // deadlockMutex guarantees only one async function is in this loop at a time, so we can't deadlock
      resourceToken = resourceMutex.lock(resourceToken);
      someSyncOperationWhichNeedsTheLock();
      if (unlikely()) await someAsyncOperationWhichNeedsTheLock();
      resourceToken.unlock();
      someExpensiveComputation(); // we want to allow other threads to use the resource while this happens
    }
    deadlockToken.unlock();
    await someOtherOperation();
  } catch (e) {
    handle(e);
  } finally {
    // make sure we release the locks when the function is done even in case of errors
    deadlockToken.unlock();
    resourceToken.unlock();
  }
}

await Promise.all([foo(1), foo(2)]);

In this example, this costs only four object allocations (2 mutexes * 2 calls to foo), because you're re-using the token object in the critical section where the repeated locking happens. The allocation of resourceToken happens only the first time through the loop, where resourceToken is undefined.

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

I think I'm okay with the reusable token idea. In the limit, for apps that want to minimize costs here, the cost is a constant one (1) additional allocation per Mutex. That is, the unsafe mutex API can be recovered by using (mutex, token) pairs where you reuse the same token.

@syg
Copy link
Collaborator

syg commented Sep 18, 2024

Coming back to Ron's UniqueLock. Isn't that wrapper buildable even with tokens? I don't see competing designs here. The token idea is for the underlying lockable (mutex), while the UniqueLock API is an abstraction on top.

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

I think it's buildable on top of the token design, yes, as long as its unlock method respects the constraint that it only works if the mutex is locked because of a previous call to .lock on this specific lock. In which case it's basically just a wrapper holding a mutex and a token for it, and adding some methods.

Strictly as a question of design I am not currently convinced it should be built into the standard library, though.

syg added a commit that referenced this issue Sep 18, 2024
Uses the reusable token-based design from #27
@rbuckton
Copy link
Collaborator

Let me walk back my position somewhat. One of the main motivators for UniqueLock was to have a mechanism we could leverage for locking if shared objects couldn't have per-realm prototypes. I'm generally fine with lock tokens, I just find the somewhat simplistic design to be a bit of an impedance mismatch vs. some of the advanced use cases for shared memory multithreading we're seeking to support.

How do we see Mutex and Condition interacting with each other? The general principle is that condition.wait accepts a locked mutex, unlocks it, waits to be signaled, reacquires the lock, and then returns. However, if we extend this to support a waitAsync mechanism, you could once again reference the same mutex in the wrong state.

As such, I would expect the condition to instead accept the lock token, which it would need to be able to unlock and reacquire. That either means we need a way to programmatically unlock and relock a lock token, or we need to handle that internally within the spec by giving Condition privileged access to the underlying state behind the lock.

How would you anticipate that mutex.lock(existingToken) should interact with condition.wait(existingToken)? You don't want to reuse a token that is holding an open lock, but you also don't want to reuse a token that is currently unlocked as part of .wait().

What do you expect would happen if mutex2 reuses a lock token originally produced by mutex1, assuming the token was released? Would it become bound to mutex2 or throw?

UniqueLock also has these concerns, obviously. I favor UniqueLock because it treats the lock token as more of a first-class capability rather than a byproduct of the locking mechanism. If a lock token can be reused between two different mutexes, then it could potentially be completely unbound from either mutex. You could think of UniqueLock as the lock token itself. It could be created bound to a mutex, lock and unlock that mutex, detached from that mutex and reused with a different mutex. The difference between UniqueLock and a lock token with these capabilities is that you could invoke new UniqueLock() to get token in an unbound state.

@bakkot
Copy link
Author

bakkot commented Sep 18, 2024

OK, it sounds like we're mostly on the same page, in that case. Specifically

The difference between UniqueLock and a lock token with these capabilities is that you could invoke new UniqueLock() to get token in an unbound state.

I am happy to have new UnlockToken available to give you an unbound token, and to allow re-binding existing unlocked tokens to other mutexes than those they've been attached to previously. I don't see any reason to forbid that.


How do we see Mutex and Condition interacting with each other? [...] As such, I would expect the condition to instead accept the lock token, which it would need to be able to unlock and reacquire.

Given that the mutex has the lock capability, and the token has the unlock capability, the obvious consequence is that the condition would require both: basically an atomic (mutex, token) => token.unlock(); return mutex.lock(token) plus an assertion that the token is currently holding the lock for that mutex. This assertion would currently require access to some internal state, though we could easily have a mutex.isThisTheTokenIAmCurrentlyLockedOn(token) method if we want to expose that capability to userland.

For the async case (or even the sync case, if tokens are cloneable to other threads, which they probably should be), this raises the question of what happens if someone re-binds the token while the condition variable is waiting to re-acquire the token. I see two possibilities: either the wait throws and unlocks the token after it acquires the lock and sees that this has happened, or we have a "third state" for tokens which means "not currently locked but not available for re-use", and the condition variable would put the token in this state while waiting. Either's fine; the second option gives better errors but may not be worth the additional complexity.


I think the remaining major differences between UniqueLock and the design I'm proposing are the following:

  • in mine, the lock capability is on the mutex, not the token
  • locking is always done by calling methods on the mutex, not by constructing a token or calling methods on one
  • there is no notion of an "associated mutex" for a token except while the token holds the lock, and consequently no release
  • there is no swap/move (this is orthogonal, I think)
  • holding the token doesn't give you access to the mutex

@bakkot
Copy link
Author

bakkot commented Sep 19, 2024

I could probably be convinced that it's OK for condition variables to magically have the ability to unlock a mutex given only the token which currently locks that mutex, instead of needing to explicitly pass in both the mutex and its token.

@bakkot
Copy link
Author

bakkot commented Sep 19, 2024

OK I see we have re-invented Rust's design again: MutexGuard (their unlock token) doesn't give access to the underlying mutex (as far as I can tell), but Condvar's wait only takes a MutexGuard and it works by reaching into the guard's internals to get at the mutex.

Their tokens can't be shared so you don't have the issue of someone re-using the token for something else while you're waiting.

@syg
Copy link
Collaborator

syg commented Sep 19, 2024

I could probably be convinced that it's OK for condition variables to magically have the ability to unlock a mutex given only the token which currently locks that mutex, instead of needing to explicitly pass in both the mutex and its token.

Yeah that's what I did in the spec draft.

@syg
Copy link
Collaborator

syg commented Sep 19, 2024

How would you anticipate that mutex.lock(existingToken) should interact with condition.wait(existingToken)? You don't want to reuse a token that is holding an open lock, but you also don't want to reuse a token that is currently unlocked as part of .wait().

This point is important as it points out that shareable tokens add back the same problems but with even more complexity.

I think the conclusion is that tokens must not be shareable, but instead must be moveable. For a lock, the owner is a thread of execution, so the problem arises if multiple threads of execution all have access to an unlock capability. This all works fine (I think?), if, on postMessage, the unlock token from the sender thread is detached.

Edit: That's a big if, actually, on if it's feasible to directly hand lock ownership from one thread to another. For manually written ParkingLot-style mutexes, I think it's probably fine? But if it isn't, I can also be convinced the ability to move lock ownership across threads is just not a common enough use case to bend over backwards to enable.

@syg
Copy link
Collaborator

syg commented Sep 19, 2024

What do you expect would happen if mutex2 reuses a lock token originally produced by mutex1, assuming the token was released? Would it become bound to mutex2 or throw?

It's re-bound to mutex2. Is this an issue?

syg added a commit that referenced this issue Sep 20, 2024
Uses the reusable token-based design from #27
@rbuckton
Copy link
Collaborator

What do you expect would happen if mutex2 reuses a lock token originally produced by mutex1, assuming the token was released? Would it become bound to mutex2 or throw?

It's re-bound to mutex2. Is this an issue?

No, it just informs the design. If lock tokens cannot be rebound then the only thing that could produce them would be the Mutex. If they can be rebound, or can be bound to nothing, then it might make more sense to be able to create them initially unbound, in which case you might want an independent constructor to avoid having to do something like:

// throw away mutex
const mut = new Atomics.Mutex();
const token = mut.lock();
token.unlock();

just to get an unbound lock token for reuse.

@syg
Copy link
Collaborator

syg commented Sep 22, 2024

If lock tokens cannot be rebound then the only thing that could produce them would be the Mutex

Agreed. The spec draft in #30 adds Atomics.Mutex.UnlockToken as a constructor to create empty tokens.

@rbuckton
Copy link
Collaborator

I'm not a fan of the name UnlockToken, but we can bikeshed that in Stage 2.

syg added a commit that referenced this issue Sep 26, 2024
Uses the reusable token-based design from #27
@syg
Copy link
Collaborator

syg commented Sep 26, 2024

This is the approach in the spec draft, so closing.

@syg syg closed this as completed Sep 26, 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

No branches or pull requests

4 participants