From edfecbb97539fcf87dfcc9fc59997894f4e0bb34 Mon Sep 17 00:00:00 2001 From: Ben Caimano <bcaimano@cloudflare.com> Date: Mon, 13 Feb 2023 14:15:56 -0500 Subject: [PATCH] Allow setAlarm to be invoked in Durable Object constructors For implementation reasons, we cannot confirm the presence of an alarm handler during the invocation of a durable object's constructor. That said, we also cannot confirm the absence of that handler either. Let's just lean on the alarm runner to do the right thing when the time comes. --- src/workerd/api/actor-state.c++ | 16 +++++++++------- src/workerd/io/worker.c++ | 29 +++++++++++++++++++++++++++-- src/workerd/io/worker.h | 2 +- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/workerd/api/actor-state.c++ b/src/workerd/api/actor-state.c++ index 171c47bfb7a..b491f0a81c7 100644 --- a/src/workerd/api/actor-state.c++ +++ b/src/workerd/api/actor-state.c++ @@ -231,11 +231,8 @@ jsg::Promise<jsg::Value> DurableObjectStorageOperations::getOne( jsg::Promise<kj::Maybe<double>> DurableObjectStorageOperations::getAlarm( jsg::Optional<GetAlarmOptions> maybeOptions, v8::Isolate* isolate) { - - if (!IoContext::current().getActorOrThrow().hasAlarmHandler()) { - return jsg::resolvedPromise<kj::Maybe<double>>(isolate, nullptr); - } - + // Even if we do not have an alarm handler, we might once have had one. It's fine to return + // whatever a previous alarm setting or a falsy result. auto options = configureOptions(maybeOptions.map([](auto& o) { return GetOptions { .allowConcurrency = o.allowConcurrency, @@ -400,8 +397,11 @@ jsg::Promise<void> DurableObjectStorageOperations::setAlarm(kj::Date scheduledTi JSG_REQUIRE(scheduledTime > kj::origin<kj::Date>(), TypeError, "setAlarm() cannot be called with an alarm time <= 0"); - JSG_REQUIRE(IoContext::current().getActorOrThrow().hasAlarmHandler(), TypeError, - "Your Durable Object class must have an alarm() handler in order to call setAlarm()"); + // This doesn't check if we have an alarm handler per say. It checks if we have an initialized + // (post-ctor) JS durable object with an alarm handler. Notably, this means this won't throw if + // `setAlarm` is invoked in the DO ctor even if the DO class does not have an alarm handler. This + // is better than throwing even if we do have an alarm handler. + IoContext::current().getActorOrThrow().assertCanSetAlarm(); auto options = configureOptions(maybeOptions.map([](auto& o) { return PutOptions { @@ -470,6 +470,8 @@ kj::OneOf<jsg::Promise<bool>, jsg::Promise<int>> DurableObjectStorageOperations: jsg::Promise<void> DurableObjectStorageOperations::deleteAlarm( jsg::Optional<SetAlarmOptions> maybeOptions, v8::Isolate* isolate) { + // Even if we do not have an alarm handler, we might once have had one. It's fine to remove that + // alarm or noop on the absence of one. auto options = configureOptions(maybeOptions.map([](auto& o) { return PutOptions { .allowConcurrency = o.allowConcurrency, diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index 5d2744b3927..222b6a9dba3 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -2821,8 +2821,33 @@ kj::Maybe<jsg::Ref<api::DurableObjectStorage>> }); } -bool Worker::Actor::hasAlarmHandler() { - return getHandler().map([](auto& h) { return h.alarm != nullptr; }).orDefault(false); +void Worker::Actor::assertCanSetAlarm() { + KJ_SWITCH_ONEOF(impl->classInstance) { + KJ_CASE_ONEOF(_, Impl::NoClass) { + // Once upon a time, we allowed actors without classes. Let's make a nicer message if we + // we somehow see a classless actor attempt to run an alarm in the wild. + JSG_FAIL_REQUIRE(TypeError, + "Your Durable Object must be class-based in order to call setAlarm()"); + } + KJ_CASE_ONEOF(_, DurableObjectConstructor*) { + KJ_FAIL_ASSERT("setAlarm() invoked before Durable Object ctor"); + } + KJ_CASE_ONEOF(_, Impl::Initializing) { + // We don't explicitly know if we have an alarm handler or not, so just let it happen. We'll + // handle it when we go to run the alarm. + return; + } + KJ_CASE_ONEOF(handler, api::ExportedHandler) { + JSG_REQUIRE(handler.alarm != nullptr, TypeError, + "Your Durable Object class must have an alarm() handler in order to call setAlarm()"); + return; + } + KJ_CASE_ONEOF(exception, kj::Exception) { + // We've failed in the ctor, might as well just throw that exception for now. + kj::throwFatalException(kj::cp(exception)); + } + } + KJ_UNREACHABLE; } kj::Promise<void> Worker::Actor::makeAlarmTaskForPreview(kj::Date scheduledTime) { diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index de450ab1cc8..bb8d06f74e9 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -703,7 +703,7 @@ class Worker::Actor final: public kj::Refcounted { const Worker& getWorker() { return *worker; } - bool hasAlarmHandler(); + void assertCanSetAlarm(); kj::Promise<void> makeAlarmTaskForPreview(kj::Date scheduledTime); kj::Promise<WorkerInterface::AlarmResult> dedupAlarm(