document the locks that were always there, and add groups - #652
Merged
Conversation
a reader of docs/concurrency.md would conclude that pith gives you channels and nothing else for shared state. Mutex, AtomicInt, WaitGroup and Semaphore have been global builtins the whole time — no import, and std.log, std.metrics, std.trace, std.io, std.uuid, std.net.tls and both database drivers are built on them — but nothing outside the standard library's own source said so. they were discoverable only by reading std. so the larger half of this is documentation: what each one is for, and the parts that bite. do not hold a mutex across a channel receive or a socket read, since a parked task holds it for as long as it is parked. there is no reentrancy. store(load() + 1) is two operations and loses increments under contention — compare_set in a loop is the fix. that last one is not theoretical. the first draft of the regression case here used the short form and printed 4 completions instead of 5 about one run in five, and the same pattern in the test fixtures would have flaked in ci. the genuinely missing piece was a group. concurrent.group(parent) fans work out and fails as a unit: the first failure cancels the group's context so siblings can stop early, and wait() reports that first error while still awaiting every task, so nothing is left running when it returns. group_limited caps how many run at once, for a fan-out wider than the resource behind it. they are documented rather than re-exported through std.concurrent. the types are global builtins; wrapping them in a module that hands back the same global type would add an import for no type safety and leave two ways to do one thing. the gap was that nobody could find them, and that is a documentation gap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
prompted by: "there's no mutex, no atomic, no waitgroup/errgroup, no semaphore as public API — the machinery exists but isn't exported. anyone writing a stateful gRPC service (a chat room registry, say) has channels and nothing else."
the experience is right; the diagnosis turned out to be wrong in a way that changes the fix.
Mutex(),AtomicInt(n),WaitGroup()andSemaphore(n)are global builtins, not internal-and-unexported. they are callable from any pith program with no import, andstd.log,std.metrics,std.trace,std.io,std.uuid,std.net.tls,std.prometheusand both database drivers are built on them.but nothing outside std's own source said so.
docs/concurrency.mdhad a "sharing between tasks" section that told you to pass data through channels and never mentioned one of them. so "channels and nothing else" was an accurate reading of everything a user can actually see, and that made the fix mostly documentation rather than mostly code.what changed
docs. a new section covering all four, with the parts that bite: don't hold a mutex across a channel receive or socket read (a parked task holds it while parked), no reentrancy, and
store(load() + 1)is two operations that loses increments —compare_setin a loop when the new value depends on the old.that last one is not theoretical. the first draft of the regression case here used the short form and printed 4 completions instead of 5 about one run in five; the same pattern in the test fixtures would have flaked in ci. both now use a compare_set loop and say why.
concurrent.group. the genuinely missing piece.group(parent)fans work out and fails as a unit — the first failure cancels the group's context so siblings can stop early, andwait()reports that first error while still awaiting every task, so nothing from the group is still running when it returns.group_limited(parent, n)caps concurrency for a fan-out wider than the resource behind it. work isfn() -> Int!, since a group cares which unit failed rather than what the survivors returned.examples/room_registry.pith. the stateful service the feedback named: a registry several tasks read and write under a mutex, a join counter as an atomic, a capped flush, and a group over the rooms where one fails.a judgement call
they are documented rather than re-exported through
std.concurrent. the types are global builtins, so aconcurrent.mutex()handing back the same globalMutexwould add an import for no type safety and leave two ways to do one thing. the gap was that nobody could find them; that is a documentation gap, and widening the api surface would not have closed it. happy to add the wrappers if you'd rather have one import for all of it.what was tested
std/concurrent.pith(6 new), stable across 6 consecutive runs. they live there rather than intests/cases/because the cases runner usespith run, which executesmain()and skipstestblocks — assertions put there would never have run.tests/cases/test_concurrent_group.pith, added toMEMCHECK_CASES: 12 consecutive runs byte-identical, 3 valgrind runs clean. it covers what the colocated tests can't — result-carrying task handles and fn-values crossing a spawn on the cranelift path.examples/room_registry.pith: 10 consecutive runs byte-identical.notes
spawninside animplmethod cannot loadself— it fails in the ir consumer withunknown load source 'self' in std_concurrent___spawn_4, naming an internal symbol and no source location.Group.goreads the fields into locals first, with a comment. worth fixing separately; the workaround is fine but the error is not discoverable.