Run alarm without syncing alarm manager if currentTime past local time #5764
Conversation
Historically, if a SQLite DOs local alarm time in SQLite didn't match the AlarmManager's alarm time, we would reschedule the alarm and try to run the alarm again later. However, if the time we are attempting to sync to is already before the current real system time, there is no point in rescheduling to run later, since we might as well run now. This commit runs the alarm immediately if the current system time is already past the time we are trying to sync the alarm manager to, since it's better to run an alarm a little late rather than very late.
MellowYarker
force-pushed
the
milan/STOR-4832
branch
from
7f13363 to
4d1e592
Compare
| static constexpr kj::Date fourMs = 4 * kj::MILLISECONDS + kj::UNIX_EPOCH; | ||
| static constexpr kj::Date fiveMs = 5 * kj::MILLISECONDS + kj::UNIX_EPOCH; | ||
| // Used as the "current time" parameter for armAlarmHandler in tests. | ||
| // Set to epoch (before all test alarm times) so existing tests aren't affected by |
There was a problem hiding this comment.
Making sure existing tests aren't affected is great, but is this new behavior worth covering with a new test case?
| // If the local alarm time is already in the past, just run the handler now. This avoids | ||
| // blocking alarm execution on storage sync when storage is overloaded. The alarm will | ||
| // either delete itself on success or reschedule on failure. | ||
| if (localAlarmState != kj::none && willFireEarlier(localAlarmState, currentTime)) { |
There was a problem hiding this comment.
Given that this code block appears to be identical both here and below, can it just be moved up above the if (willFireEarlier(scheduledTime, localAlarmState)) { ... } else { ...} block rather than being repeated in both the if and the else?
| } | ||
| } | ||
|
|
||
| auto currentTime = kj::systemPreciseCalendarClock().now(); |
There was a problem hiding this comment.
It would probably be more consistent with other alarm operations to use context.now() here -- basically using the context's Date.now() value instead of the CPU clock. That's e.g. the value we use to force alarm times to the present, in DurableObjectStorageOperations::setAlarm(), as well as to ensure alarms do not run until after Date.now() becomes current, in handleAlarm().
| KJ_ASSERT_NONNULL(localAlarmState), currentTime, actorId); | ||
| haveDeferredDelete = true; | ||
| inAlarmHandler = true; | ||
| static const DeferredAlarmDeleter disposer; |
There was a problem hiding this comment.
Might be a little neater to just have a single DeferredAlarmDeleter declaration at the top of the function.
| LOG_WARNING_PERIODICALLY( | ||
| "NOSENTRY SQLite alarm overdue, running despite stale AlarmManager time", | ||
| scheduledTime, localTime, currentTime, actorId); | ||
| haveDeferredDelete = true; |
There was a problem hiding this comment.
I notice that the other existing code that sets haveDeferredDelete = true is also setting deferredAlarmSpan = kj::mv(parentSpan);... I'm not familiar with exactly how the spans are being used, but it seems possible the consuming code may expect the span to similarly be set for new cases of haveDeferredDelete = true?
| // If the local alarm time is already in the past, just run the handler now. This avoids | ||
| // blocking alarm execution on storage sync when storage is overloaded. The alarm will | ||
| // either delete itself on success or reschedule on failure. | ||
| if (localAlarmState != kj::none && willFireEarlier(localAlarmState, currentTime)) { |
There was a problem hiding this comment.
Strictly speaking, it isn't necessary to check localAlarmState != kj::none, since willFireEarlier(localAlarmState, currentTime) will never be true for localAlarmState == kj::none.
| LOG_WARNING_PERIODICALLY( | ||
| "NOSENTRY SQLite alarm overdue, running despite AlarmManager mismatch", scheduledTime, | ||
| KJ_ASSERT_NONNULL(localAlarmState), currentTime, actorId); |
There was a problem hiding this comment.
If execution reaches this point, then we know (scheduledTime < localAlarmState < currentTime). But we have code in the alarm caller that explicitly waits until Date.now() <= scheduledTime... which I think means that this clause should be theoretically unreachable, if we populate currentTime from Date.now() rather than from the CPU clock.
Ah, misread... the calling code is actually waiting until scheduledTime <= Date.now(), so should be reachable.
| // If the local alarm time is already in the past, just run the handler now. This avoids | ||
| // blocking alarm execution on storage sync when storage is overloaded. |
There was a problem hiding this comment.
The comments mention avoiding blocks on "storage sync", which I assume means being blocked on the persistence of sqlite data? In which case, it might be worth noting that this code is inside of a check for "if (localAlarmState == lastConfirmedAlarmDbState)", so should only run in the cases where persistence of alarm changes has completed -- or the unlikely case where the alarm time was changed, then quickly changed back. So might not actually help much in cases where "storage is overloaded"?
| kj::Date scheduledTime, SpanParent parentSpan, bool noCache, kj::StringPtr actorId) { | ||
| kj::Date scheduledTime, | ||
| SpanParent parentSpan, | ||
| kj::Date /*currentTime -- unused*/, |
There was a problem hiding this comment.
Curious... was this required by some compiler warning/error, or is it just for notation? I'm not seeing other code using this convention.
In either case, tagging the parameter with KJ_UNUSED or [[maybe_unused]] might also work? ...although I don't see other code doing that, either. It looks like the kj style guide just recommends compiling with -Wno-unused-parameter.
5 participants
Historically, if a SQLite DOs local alarm time in SQLite didn't match the AlarmManager's alarm time, we would reschedule the alarm and try to run the alarm again later.
However, if the time we are attempting to sync to is already before the current real system time, there is no point in rescheduling to run later, since we might as well run now.
This commit runs the alarm immediately if the current system time is already past the time we are trying to sync the alarm manager to, since it's better to run an alarm a little late rather than very late.