std.Io.Condition.wait cancelation race / error swallowing #35564
lukasl/zig:condition-wait-cancel-race into master
When std.Io.Condition is signaled and almost immediately after
that canceled, it will swallow the error.Canceled error and
leave the task in an uncancelable state.
6c053d95ff
efccd192d5
efccd192d5
dd80d9c042
This is the repro case, but I'm not sure how to extract a minimal test here:
@ -1741,1 +1741,3 @@
// We successfully consumed a signal.
// We successfully consumed a signal. If the futex wait was
// canceled, preserve that cancellation state.
resultcatchio.recancel();
I would have thought result catch |err| return err; would be more-appropriate than recancel here?
If we were the only waiter and we have been signaled, we need to consume the signal, otherwise it might get lost. The CAS above is about checking if this task was the one signaled.
If we wanted to avoid the recancel, we would essentially do something like:
result catch |err| { signal(io); return err; }
To wake the replacement for us.
We can't break the rule that there are waiter >= signals.
Hm, maybe the signal forwarding and returning error.Canceled is better. It's slightly more expensive, but allows the task to exit early, not wait for the next cancelation point.
@ -1742,0 +1741,4 @@
// We successfully consumed a signal. If the futex wait was
// canceled, forward the signal to the next waiter.
resultcatch|err|{
cond.signal(io);
Alternative form of this:
diff --git a/lib/std/Io.zig b/lib/std/Io.zig
index 8d8d31dc47..170863aa22 100644
--- a/lib/std/Io.zig
+++ b/lib/std/Io.zig
@@ -1732,19 +1732,22 @@ pub const Condition = struct {
// Even on error, try to consume a pending signal first. Otherwise a race might
// cause a signal to get stuck in the state with no corresponding waiter.
{
+ const canceled = if (result) |_| false else |_| true;
var prev_state = cond.state.load(.monotonic);
while (prev_state.signals > 0) {
+ const new_signals = if (canceled)
+ @min(prev_state.signals, prev_state.waiters - 1)
+ else
+ prev_state.signals - 1;
prev_state = cond.state.cmpxchgWeak(prev_state, .{
.waiters = prev_state.waiters - 1,
- .signals = prev_state.signals - 1,
+ .signals = new_signals,
}, .acquire, .monotonic) orelse {
- // We successfully consumed a signal. If the futex wait was
- // canceled, forward the signal to the next waiter.
- result catch |err| {
- cond.signal(io);
- return err;
- };
- return;
+ if (canceled and prev_state.signals < prev_state.waiters) {
+ _ = cond.epoch.fetchAdd(1, .release);
+ io.futexWake(u32, &cond.epoch.raw, 1);
+ }
+ return result;
};
}
}
Reproduction:
test"Condition.wait does not swallow cancelation when racing a signal"{if(builtin.single_threaded)returnerror.SkipZigTest;constio=testing.io;constContext=struct{mutex:Io.Mutex=.init,cond:Io.Condition=.init,stop:std.atomic.Value(bool)=.init(false),fnsignaler(ctx:*@This())void{while(!ctx.stop.load(.monotonic))ctx.cond.signal(io);}fnwaiter(ctx:*@This())Io.Cancelable!void{tryctx.mutex.lock(io);deferctx.mutex.unlock(io);while(true){tryctx.cond.wait(io,&ctx.mutex);}}};varctx:Context=.{};varsignaler_future=io.concurrent(Context.signaler,.{&ctx})catch|err|switch(err){error.ConcurrencyUnavailable=>returnerror.SkipZigTest,};defer{ctx.stop.store(true,.monotonic);signaler_future.await(io);}vari:usize=0;while(i<1000):(i+=1){varwaiter_future=io.concurrent(Context.waiter,.{&ctx})catch|err|switch(err){error.ConcurrencyUnavailable=>returnerror.SkipZigTest,};if(waiter_future.cancel(io))|_|{returnerror.WaiterWasNotCanceled;}else|err|switch(err){error.Canceled=>{},}}}Didn't commit it, because the success is that it doesn't hang, so it probably doesn't belong to the codebase.
No due date set.
No dependencies set.
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?