fix: timers wake immediately on actor shutdown in threads module#142
fix: timers wake immediately on actor shutdown in threads module#142ElFantasma merged 13 commits intomainfrom
Conversation
…mer-actor-shutdown
|
@Kimi please review |
|
/claude /kimi /codex |
PR Review - FindingsCRITICAL: Race Condition in CancellationTokenSeverity: High The pub fn on_cancel(&self, callback: CancelCallback) {
let mut callbacks = self.callbacks.lock().unwrap_or_else(|e| e.into_inner());
if self.is_cancelled() { // ← Race window here\!
drop(callbacks);
callback();
} else {
callbacks.push(callback);
}
}The Problem:
The Fix: pub fn cancel(&self) {
let mut callbacks = self.callbacks.lock().unwrap_or_else(|e| e.into_inner());
if self.is_cancelled.swap(true, Ordering::SeqCst) {
return; // Already cancelled
}
let cbs: Vec<_> = callbacks.drain(..).collect();
drop(callbacks); // Release lock before firing
for cb in cbs {
cb();
}
}
pub fn on_cancel(&self, callback: CancelCallback) {
let mut callbacks = self.callbacks.lock().unwrap_or_else(|e| e.into_inner());
if self.is_cancelled() {
drop(callbacks);
callback();
} else {
callbacks.push(callback);
}
}This ensures the flag is set while holding the lock, preventing the race. BUG: Incorrect Loop Logic in send_intervalSeverity: Medium The loop exits when cancellation is detected AFTER a timeout, but it should send the message BEFORE checking cancellation to avoid skipping the final message: while let Err(RecvTimeoutError::Timeout) = wake_rx.recv_timeout(period) {
if timer_token.is_cancelled() || actor_token.is_cancelled() {
break; // ← Checks BEFORE sending
}
let _ = handle.send(message.clone());
}This means if cancellation happens just as the timeout expires, no message is sent. Consider checking after the send, or document this behavior clearly. Minor Issues
Positive Changes
Automated review by Claude (Anthropic) |
|
/kimi |
|
/codex |
🤖 Kimi Code ReviewSecurity & Bug Review1. Critical Security Issue –
|
🤖 Codex Code ReviewFindings
No other security vulnerabilities or significant bugs found in the PR diff. Automated review by OpenAI Codex |
Analysis of AI Review Comments1. Claude & Kimi: Race condition in CancellationToken - ❌ InvalidBoth reviewers claim there's a race condition in pub fn on_cancel(&self, callback: CancelCallback) {
let mut callbacks = self.callbacks.lock()...; // Lock acquired FIRST
if self.is_cancelled() { // Check while holding lock
drop(callbacks);
callback();
} else {
callbacks.push(callback);
}
}The code comment explains the design:
Claude's scenario ("Thread B acquires lock, drains callbacks") is impossible while Thread A holds the lock. The callback is guaranteed to fire exactly once. 2. Claude & Kimi: send_interval logic flaw - ❌ InvalidCurrent code checks cancellation BEFORE sending: if timer_token.is_cancelled() || actor_token.is_cancelled() {
break; // Don't send if cancelled
}
let _ = handle.send(message.clone());This is correct behavior. If cancelled, we should NOT send. The reviewers suggest swapping the order, which would incorrectly send messages after cancellation. 3. Codex: init_tracing() panics if called twice -
|
| Issue | Reviewer | Verdict |
|---|---|---|
| Race condition in on_cancel | Claude, Kimi | ❌ Invalid - lock prevents race |
| send_interval logic | Claude, Kimi | ❌ Invalid - current order is correct |
| init_tracing panic | Codex | |
| ctrl_c memory leak | Codex | ✅ Fixed in PR #140 |
| is_cancelled() change | Kimi |
concurrency/src/threads/stream.rs
Outdated
| { | ||
| let mut iter = stream.into_iter(); | ||
| let mut cancelation_token = handle.cancellation_token(); | ||
| let cancelation_token = handle.cancellation_token(); |
There was a problem hiding this comment.
there is a type: cancelation_token has 2 l's
2 participants
EDIT (28-01-2026):
Ran Devin review and fixed all found issues:
https://app.devin.ai/review/lambdaclass/spawned/pull/142
Summary
on_cancel()callback toCancellationTokenfor efficient waitingis_cancelled()bug that was clearing the flagCancellationTokenmethods from&mut selfto&selfsend_after/send_intervalto use channel-based wake patterntest_send_after_actor_shutdowntestCloses #123