Rewrite durable object alarm deduplication with cancellation considerations #726
Conversation
bcaimano
requested review from
jclee,
xortive,
jqmmes,
a-robinson and
MellowYarker
bcaimano
force-pushed
the
bcaimano/run-alarm
branch
2 times, most recently
from
86484be to
8cc0ee6
Compare
There was a problem hiding this comment.
I'd feel a lot better about @xortive reviewing this than me, but this LGTM
| } | ||
|
|
||
| KJ_IF_MAYBE(scheduledAlarm, impl->maybeScheduledAlarm) { | ||
| // We had a previously scheduled alarm, let's cancel it. |
There was a problem hiding this comment.
What if it was for the same time? Do we really want to cancel it even in that case or would we be better off just adding a branch to its result promise?
There was a problem hiding this comment.
So the only caller of scheduleAlarm() calls getAlarm() in the same synchronous scope, essentially doing what you describe. I'll add the comment in the header!
|
|
||
| co_return co_await whenReady.then([this]() -> ScheduleAlarmResult { | ||
| // It's time to run! Let's tear apart the scheduled alarm and make a running alarm. | ||
| auto scheduledAlarm = KJ_ASSERT_NONNULL(kj::mv(impl->maybeScheduledAlarm)); |
There was a problem hiding this comment.
So we don't need to verify that impl->maybeScheduledAlarm still contains the expected scheduledTime? Because whenCanceled would definitely be resolved first (before whenReady) if scheduledAlarm had been removed/changed?
That looks like it's probably true, I just haven't convinced myself it -- so mostly I'm asking for reassurance that you have convinced yourself.
There was a problem hiding this comment.
Yep, you have it right, but that deserves a comment!
bcaimano
force-pushed
the
bcaimano/run-alarm
branch
from
8cc0ee6 to
3ff03a5
Compare
…ations This commit does a few notable things: - The WorkerEntrypoint now invokes `context.run()` and `incomingRequest.drain()` on the promise chain for the `runAlarm()` request. This means that we are able to report to the actor when that request is cancelled, which implies that the worker entrypoint will be going away. (There is an explicit guard for request cancellation now.) - The Worker::Actor now has two functions for alarms. `getAlarm()` checks if there is an existing alarm for a given time synchronously. `scheduleAlarm()` provides a promise that will resolve when it is time to run the alarm or the alarm has been cancelled. It also cancels the currently scheduled alarm if there is one. - Durable object alarms in the sandbox can now be "scheduled" or "running" instead of "running" or "queued". This is meaningful because "running" alarms cannot be cancelled and have associated metrics events but "scheduled" alarms can be cancelled and have no metrics events. Taken together, these changes attempt to ensure that there is a live worker entrypoint associated with each scheduled or running alarm. This can be especially important because other worker entrypoints may be waiting upon the result from that alarm.
bcaimano
force-pushed
the
bcaimano/run-alarm
branch
from
3ff03a5 to
1008d6c
Compare
This commit does a few notable things:
context.run()andincomingRequest.drain()on the promise chain for therunAlarm()request. This means that we are able to report to the actor when that request is cancelled, which implies that the worker entrypoint will be going away. (There is an explicit guard for request cancellation now.)getAlarm()checks if there is an existing alarm for a given time synchronously.scheduleAlarm()provides a promise that will resolve when it is time to run the alarm or the alarm has been cancelled. It also cancels the currently scheduled alarm if there is one.Taken together, these changes attempt to ensure that there is a live worker entrypoint associated with each scheduled or running alarm. This can be especially important because other worker entrypoints may be waiting upon the result from that alarm.