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

Normative: Add Mutex and Condition #30

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Normative: Add Mutex and Condition #30

wants to merge 7 commits into from

Conversation

syg
Copy link
Collaborator

@syg syg commented Sep 18, 2024

Uses the reusable token-based design from #27

cc @bakkot

Copy link

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

Quick skim of the mutex API mostly looks good, some comments. Didn't review memory model stuff at all.

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
1. Return UnlockTokenCreateIfNeeded(_unlockToken_, _mutex_).
1. Else,
1. Assert: _result_ is ~timed-out~.
1. Return *"timed-out"*.
Copy link

@bakkot bakkot Sep 18, 2024

Choose a reason for hiding this comment

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

I expect we'll eventually make tokens disposable, which makes the common pattern for timeouts look like

using token = mutex.lock(null, timeout);

Returning a string here means that will throw, which is... probably the thing you want? It'll be kind of a cryptic error message, but that's better than thinking you have the lock when you don't (using token = null is legal, so returning null here would make that silently succeed).

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
</emu-clause>

<emu-clause id="sec-atomics.mutex.withlock">
<h1>Atomics.Mutex.withLock ( _mutex_, _callback_ [ , _thisArg_ ] )</h1>
Copy link

Choose a reason for hiding this comment

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

Do we want tryWithLock? Not sure what to return in the already-locked case, though. Maybe have a parameter to use for that value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see that often in other PLs/stdlibs. I'm not against it though.

Copy link

Choose a reason for hiding this comment

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

Swift 6 (new as of 2 days ago) has withLockIfAvailable1, which is this. I also think that's a better name than tryLock / tryWithLock, incidentally.

Really it's just filling out the missing quadrant on the (lock()+unlock() vs callback) x (blocking vs only-if-available) matrix, though, so it's pretty natural if we already have the other three quadrants filled out.

Footnotes

  1. You can read through its history in this thread if you want to.

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated
</emu-clause>

<emu-clause id="sec-mutexunlocktokenprototype">
<h1>The %MutexUnlockTokenPrototype% Object</h1>
Copy link

Choose a reason for hiding this comment

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

Should this be exposed/constructible? Could be nice for some patterns when re-using tokens, instead of needing to wait until you actually want to lock.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't mind, but where would you put it?

Copy link

Choose a reason for hiding this comment

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

My first instinct is, if the tokens can only be used with mutexes, then Atomics.Mutex.UnlockToken, otherwise Atomics.UnlockToken.

</emu-note>

<emu-clause id="sec-atomics.mutex.lock">
<h1>Atomics.Mutex.lock ( _mutex_ [ , _unlockToken_ [ , _timeout_ ] ] )</h1>
Copy link

Choose a reason for hiding this comment

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

Presumably lockAsync to be added later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, async variants are pretty complex, and TBH I'd prefer to do them as a follow-up proposal entirely.

</emu-note>

<emu-clause id="sec-atomics.mutex.lock">
<h1>Atomics.Mutex.lock ( _mutex_ [ , _unlockToken_ [ , _timeout_ ] ] )</h1>
Copy link

Choose a reason for hiding this comment

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

Also wait why is this static instead of prototype?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because shared structs can't point to unshared objects, and functions can't be shared, so having a prototype requires something like per-Realm prototypes that I have talked about in the past, together with solving the "correlation problem" (the registry thing). We're still actively discussing how to solve this issue with Mark & co, and have decided to leave it as an open design question to be settled during Stage 2. As such, the extra complexity of per-Realm prototypes is omitted from the spec draft.

If per-Realm prototypes get consensus, then this will definitely be a prototype.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(It's my interpretation that where these methods live isn't as major a semantics point as what they do.)

@bakkot
Copy link

bakkot commented Sep 19, 2024

The token use for Atomics.Condition.wait seems fine for now, but if tokens become shareable across threads, or we add a Condition.waitAsync, we'll need to think more about what happens if someone else touches the token during the wait.

@syg
Copy link
Collaborator Author

syg commented Sep 19, 2024

The token use for Atomics.Condition.wait seems fine for now, but if tokens become shareable across threads, or we add a Condition.waitAsync, we'll need to think more about what happens if someone else touches the token during the wait.

Absolutely. The current version only works for blocking waits.

@syg syg changed the title Normative: Refactor shared struct creation; Add Atomics.Mutex Normative: Add Mutex and Condition Sep 19, 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.

2 participants