Repository navigation
Conversation
|
Since last review: 0 resolved, 0 still open, 1 new. Adds the lifetime-bound annotation and refines timer scheduling for input-gate-blocked callbacks. Reviewed commit: 7b9db539 · github run |
A fired timeout stayed at the front of `timeoutTimes` until its callback
completed, and the timer task only ever waited for that front entry. When
the callback of a timeout set outside a blockConcurrencyWhile() waited
for the input gate the critical section held, no later timeout could be
armed, including the critical section's own. The critical section then
hung until its 30-second limit broke the Durable Object:
setTimeout(() => {}, 10);
await ctx.blockConcurrencyWhile(() => sleep(200)); // hangs for 30s
The timer task now waits for the earliest timeout that has not fired, and
a timeout re-arms it as soon as it fires rather than when its callback
completes. Fired timeouts still stay in `timeoutTimes` until their
callback completes, so `getNextTimeout()` and the Date.now() clamp built
on it are unchanged. Callbacks still run in order where they contend for
the same resource: timeouts outside a critical section queue at the
input gate, and the isolate lock, in the order they fired.
websocket-open-error-test hit this under TSAN load: its sleep(1500)
came due before its critical section's sleep(1000) once a 32 MiB send
delayed the critical section by more than 500ms.
Compatibility: this only changes behavior when a fired timeout's callback
is still waiting to run, which previously blocked every later timeout. We
accept the risk without a flag; the old behavior was a hang.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@ask-bonk Thanks. On the two findings:
|
ee3dc4d to
7b9db53
Compare
| KJ_IF_SOME(entry, firstUnfired()) { | ||
| // Wait for the earliest timeout that has not fired. | ||
| armedTimeout = entry.key; | ||
| timerTask = timerChannel.atTime(entry.key.when) |
There was a problem hiding this comment.
[BLOCKING] When several timer deadlines have already passed, atTime() can return kj::READY_NOW. The eager RHS can then fulfill the next timeout and re-enter resetTimerTask() before this assignment stores its own task. The nested call installs a task for the following overdue timeout, but this outer assignment immediately overwrites and cancels it while armedTimeout still names that following timeout. No later operation rearms it, so the timer can remain permanently unfulfilled and again stall a critical section. Defer fulfillment by one event-loop turn, so the eager task is installed before it can re-enter scheduling.
| timerTask = timerChannel.atTime(entry.key.when) | |
| timerTask = timerChannel.atTime(entry.key.when) | |
| .then([this, key = entry.key]() { | |
| return kj::evalLater([this, key]() { | |
| auto& newEntry = KJ_ASSERT_NONNULL(firstUnfired()); | |
| KJ_ASSERT(newEntry.key == key, | |
| "earliest timeout changed without calling resetTimerTask(), we probably missed " | |
| "a timeout!"); | |
| armedTimeout = kj::none; | |
| newEntry.value->fulfill(); | |
| }); | |
| }).eagerlyEvaluate([](kj::Exception&& e) { KJ_LOG(ERROR, e); }); |
A fired timeout stays at the front of
TimeoutManagerImpl::timeoutTimesuntil its callback completes, and the timer task only waited for that front entry. When the callback of a timeout set outsideblockConcurrencyWhile()waits for the input gate the critical section holds, no later timeout gets armed, including the critical section's own. The critical section then hangs until its 30s limit resets the Durable Object:This reproduces on main's C++ server. It's also the root cause of
websocket-open-error-test's TSAN failures in #7639: under load, itssleep(1500)comes due before the critical section'ssleep(1000).Fix: the timer task waits for the earliest timeout that hasn't fired, and a timeout re-arms it as soon as it fires rather than when its callback completes.
timeoutTimesuntil their callback completes, sogetNextTimeout()and the SpectreDate.now()clamp are unchanged.Compatibility: behavior changes only while a fired timeout's callback is still waiting to run, which used to block every later timeout. The risk is accepted without a flag, since the old behavior was a hang.
Test:
actor-block-concurrency-timer-testtimes out without the fix (15s) and passes with it. The local io/api/server suites pass, except network/sandbox-bound tests (Pyodide/DNS TLS, UDP) that fail locally regardless.🤖 Generated with Claude Code