refactor: use Future.cancel, remove 500ms chunking workaround#2
refactor: use Future.cancel, remove 500ms chunking workaround#2
Conversation
Replace the 500ms sleep-chunking workaround (for Zig bug #31307) with direct Future.cancel in join(). This allows runAutoLoop to sleep the full duration until the next slot boundary. Future.cancel segfaults on macOS GCD but may work on Linux io_uring. CI temporarily set to Linux-only to validate.
There was a problem hiding this comment.
Pull request overview
This PR refactors EventClock shutdown behavior by switching the loop fiber termination in join() from await() to Future.cancel(), and removes the 500ms sleep-chunk workaround that existed due to Zig bug #31307. It also narrows CI to Linux to validate behavior specifically on the io_uring backend.
Changes:
- Update
EventClock.join()to useFuture.cancel()instead ofawait(). - Simplify
runAutoLoopsleep behavior to sleep the full slot duration (no 500ms chunking). - Update/rename the real-time shutdown test and restrict CI workflow to
ubuntu-latest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/EventClock.zig | Switches loop shutdown strategy to Future.cancel, removes chunked sleeping, updates related test naming/comments. |
| .github/workflows/ci.yml | Runs CI only on Linux to isolate io_uring behavior while validating Future.cancel. |
Comments suppressed due to low confidence (1)
src/EventClock.zig:449
- The debug log in the
sleeperror path always says "retrying", but when the error is fromjoin()cancellation the loop will typically exit on the next condition check rather than retrying the sleep. Consider adjusting the message (or branching onself.stopped) so logs remain accurate during normal shutdown/cancel behavior.
std.Io.Duration.fromMilliseconds(sleep_ms),
.awake,
) catch |err| {
std.log.debug("EventClock: sleep failed ({s}), retrying", .{@errorName(err)});
continue;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Cancel the loop fiber and wait for it to finish. | ||
| pub fn join(self: *EventClock) void { | ||
| var maybe_future = self.loop_future; | ||
| self.loop_future = null; | ||
| if (maybe_future) |*future| { | ||
| future.await(self.io); | ||
| future.cancel(self.io); | ||
| } | ||
| } |
There was a problem hiding this comment.
join()’s docstring says it “cancel[s] the loop fiber and wait[s] for it to finish”, but the implementation only calls future.cancel(self.io) and never awaits/completes the future explicitly. If Future.cancel is not guaranteed to block until the fiber terminates, deinit() can free self’s resources while runAutoLoop is still running (use-after-free). Please either (a) ensure join() truly waits for completion (e.g., cancel then await / otherwise drain the future), or (b) adjust the doc + deinit contract to match cancel’s actual semantics.
| matrix: | ||
| os: [ubuntu-latest, macos-latest] | ||
| runs-on: ${{ matrix.os }} | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
CI is now Linux-only (runs-on: ubuntu-latest) which removes macOS coverage for this repo. If this is intended to be temporary for validating Future.cancel on io_uring, consider keeping macOS in the workflow but marking it continue-on-error or gating it behind a workflow input so platform regressions aren’t silently missed.
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| os: [ubuntu-latest, macos-latest] | |
| runs-on: ${{ matrix.os }} | |
| continue-on-error: ${{ matrix.os == 'macos-latest' }} |
Make WaitForSlotResult self-contained like Java's Future by adding a back-pointer to EventClock. Callers can now use idiomatic Zig `errdefer fut.cancel()` instead of `ec.cancelWait(&fut)`. All waitForSlot call sites (tests + example) updated to use the errdefer pattern.
Defensive cleanup per TigerStyle — don't hold a pointer to memory you no longer own. Prevents latent dangling pointer if WaitForSlotResult outlives its EventClock.
Summary
future.await()withfuture.cancel()injoin()runAutoLoop, sleep full slot duration#31307workaround commentsMotivation
The 500ms chunking was a workaround for Zig bug #31307 where
Future.cancelsegfaults on macOS GCD. This PR tests whetherFuture.cancelworks correctly on Linux io_uring.