Replace sleep-polling loops with condition-based waits

[vtjnash]

The worker synchronization paths busy-waited by calling sleep in a loop while polling shared state (worker connection state, cluster membership, and worker termination). Migrate these to wait on a Threads.Condition with proper lock/notify/wait so waiters wake promptly on the relevant change instead of polling.

• Change Worker.c_state to a Threads.Condition and take its lock around every state read/write and notify.
• Add a wait_or_deadline(cond, deadline) helper that waits for a notification up to an absolute deadline (introducing spurious notifications for all users of cond).
• Add a module-level worker_state_cond, notified in register_worker, so check_master_connect and the custom-topology setup can wait for workers/master to join rather than polling.

This is also a step towards more thread-safety, but not complete.

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.78%. Comparing base (f6cf276) to head (37c468c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/cluster.jl	93.54%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   79.43%   79.78%   +0.35%     
==========================================
  Files          10       10              
  Lines        1969     1999      +30     
==========================================
+ Hits         1564     1595      +31     
+ Misses        405      404       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JamesWrigley]

I don't really agree with this. For events that occur infrequently and with a wait time on the order of 100s of milliseconds polling is perfectly fine, and in this case objectively simpler than using conditions. timedwait() is fine sometimes 🤷

[vtjnash]

The timedwait implementation also has numerous threading bugs that this condition variable version doesn't have

[JamesWrigley]

Like what?

[vtjnash]

bugs marked as review comments. but anyways, why would we ever use a bad implementation (timedwait) when the correct implementation is so trivial to do

[vtjnash]

Failure to lock before changing state

[vtjnash]

O(n^2) loop when O(n) condition was sufficient

[vtjnash]

Access to threaded value outside of a lock

[vtjnash]

Access to a threaded value outside of a lock

[vtjnash]

Access to a threaded value outside of a lock

[vtjnash]

Unnecessary delay for more than 1 ms (for :custom topology)

[JamesWrigley]

Aren't all of those bugs unrelated to polling? Maybe except for the O(n^2) loop, which I kinda doubt will ever be an issue. To clarify, the thing that gives me pause is wait_for_deadline(). The main things I dislike are:

1. The API uses non-finite deadline values to indicate wait-forever, which is quite different from other optional-timeout APIs like poll_fd() and timedwait(). Using negative values would be less surprising I think.
2. The caller now has to handle spurious wakeups, which for these cases seem excessively complicated compared to polling.

[vtjnash]

Yes, the claim isn't directly that condition is better than timedwait, the claim is the reverse: if you're using timedwait, it probably means you are doing a bad job at handling concurrency correctly everywhere else. And that's what is seen is true here. Once you fix all the concurrency bugs here, the use of a condition variable falls out naturally, because you already had to have that to structure to fix all the other concurrency mistakes. The use of timedwait isn't just a design problem because it causes latency issues, but also because it exposes the other underlying soundness issues with the implementation.

Handling spurious wakeups are a concurrency-correctness 101 topic. I'd hardly consider a beginner-class intro lecture topic to ever be considered excessively complicated. These are simply clones of the "naive first attempt" vs "correct implementation" template examples from https://en.wikipedia.org/wiki/Monitor_(synchronization)

[vtjnash]

Aside: some languages do have an overload of the wait function which works like timedwait here in taking a lambda to evaluate to slightly reduce that boilerplate variance when exchanging timedwait for wait: https://en.cppreference.com/cpp/thread/condition_variable/wait_until. We might want to add that to our API too, just so people are more likely to implement condition variable waits without accidental typos.

The API uses non-finite deadline values to indicate wait-forever, which is quite different from other optional-timeout APIs like poll_fd() and timedwait(). Using negative values would be less surprising I think.

It isn't unheard for Julia to use Inf to mean no-limit, though often typemax is used instead (for example, Channel uses both patterns). Time is unsigned, so it cannot be negative, and I don't like that discontinuity anyways. Using deadline here vs the usual timeout value is a significant code reduction here, since every caller wanted the deadline version and has to derive a timeout from that.

[JamesWrigley]

In most situations I would also go for a condition, but here the case is not very convincing to me. Given how infrequently the events occur I don't see a performance reason for avoiding polling, and if I read the code you pointed out it's not clear to me how using a condition would prevent that thread-unsafe behaviour by design. For example, accessing map_pid_wrkr outside of a lock is indeed unsafe, but wouldn't it be best to make it a Base.Lockable instead? The condition is not attached to the dict so it's still quite possible to access it outside of a lock.

BTW for my own understanding, why is something like @atomic jw.state not threadsafe?

[vtjnash]

The read must synchronize with the c_state notify call there, which only happens if you lock before the read

Base.Lockable is also reasonable here. Orthogonal change though since both constructors (Condition and Lockable) take an argument of which lock to use internally, specifically so that they can share the same locks.

[vtjnash]

would prevent that thread-unsafe behaviour by design

I will repeat, it does not help prevent thread-unsafe behaviors by design. But the converse: thread-safe designs are trivial to make use correct notify variables because all the synchronization work is already done, while bad implementations have to resort to using timedwait because they are unsound