From 1232cc09f7aec7d5e390bc9b4992972e57362d8a Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Thu, 26 Oct 2023 15:36:43 -0400 Subject: [PATCH 1/5] Remove params data member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m going to want to put things in the SetupParams struct that must be moved out in order to be used. As such, some of the things in the params member would be valid and some things would be invalid after running the constructor. Rather than have to document this mess, it’s better to treat the SetupParams as something only the constructor has access to and save away everything else we need other data member. --- src/workerd/tests/test-fixture.c++ | 2 +- src/workerd/tests/test-fixture.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index abfcc63cd45..fbebb930c4e 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -234,7 +234,7 @@ struct MockResponse final: public kj::HttpService::Response { TestFixture::TestFixture(SetupParams params) - : params(params), + : waitScope(params.waitScope), config(buildConfig(params, configArena)), io(params.waitScope == kj::none ? kj::Maybe(kj::setupAsyncIo()) : kj::Maybe(kj::none)), timer(kj::heap()), diff --git a/src/workerd/tests/test-fixture.h b/src/workerd/tests/test-fixture.h index 6ebb2289fac..21bed9c7731 100644 --- a/src/workerd/tests/test-fixture.h +++ b/src/workerd/tests/test-fixture.h @@ -50,7 +50,7 @@ struct TestFixture { -> typename RunReturnType()))>::Type { auto request = createIncomingRequest(); kj::WaitScope* waitScope; - KJ_IF_SOME(ws, params.waitScope) { + KJ_IF_SOME(ws, this->waitScope) { waitScope = &ws; } else { waitScope = &KJ_REQUIRE_NONNULL(io).waitScope; @@ -80,7 +80,7 @@ struct TestFixture { Response runRequest(kj::HttpMethod method, kj::StringPtr url, kj::StringPtr body); private: - SetupParams params; + kj::Maybe waitScope; capnp::MallocMessageBuilder configArena; workerd::server::config::Worker::Reader config; kj::Maybe io; From be056d0c6dc9da3d2f105ce2eb8711bb2fdf5ff9 Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Fri, 27 Oct 2023 06:02:06 -0400 Subject: [PATCH 2/5] Make TestFixture constructor parameter an rvalue reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I will need to put things in the SetupParams struct that are only moveable. As such, we’ll want the params parameter to be an rvalue reference so the compiler can give us hints to move the struct into place. --- src/workerd/tests/bench-global-scope.c++ | 2 +- src/workerd/tests/bench-regex.c++ | 2 +- src/workerd/tests/test-fixture.c++ | 2 +- src/workerd/tests/test-fixture.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workerd/tests/bench-global-scope.c++ b/src/workerd/tests/bench-global-scope.c++ index dfb46240e6d..d7bffc959c5 100644 --- a/src/workerd/tests/bench-global-scope.c++ +++ b/src/workerd/tests/bench-global-scope.c++ @@ -22,7 +22,7 @@ struct GlobalScopeBenchmark: public benchmark::Fixture { }, }; )"_kj}; - fixture = kj::heap(params); + fixture = kj::heap(kj::mv(params)); } void TearDown(benchmark::State& state) noexcept(true) override { diff --git a/src/workerd/tests/bench-regex.c++ b/src/workerd/tests/bench-regex.c++ index eb278dfe01e..1daf4c1a74d 100644 --- a/src/workerd/tests/bench-regex.c++ +++ b/src/workerd/tests/bench-regex.c++ @@ -32,7 +32,7 @@ struct RegExpBenchmark: public benchmark::Fixture { } )"_kj }; - fixture = kj::heap(params); + fixture = kj::heap(kj::mv(params)); } void TearDown(benchmark::State& state) noexcept(true) override { diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index fbebb930c4e..2e3253ee420 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -233,7 +233,7 @@ struct MockResponse final: public kj::HttpService::Response { } // namespace -TestFixture::TestFixture(SetupParams params) +TestFixture::TestFixture(SetupParams&& params) : waitScope(params.waitScope), config(buildConfig(params, configArena)), io(params.waitScope == kj::none ? kj::Maybe(kj::setupAsyncIo()) : kj::Maybe(kj::none)), diff --git a/src/workerd/tests/test-fixture.h b/src/workerd/tests/test-fixture.h index 21bed9c7731..7ad094e1765 100644 --- a/src/workerd/tests/test-fixture.h +++ b/src/workerd/tests/test-fixture.h @@ -25,7 +25,7 @@ struct TestFixture { kj::Maybe mainModuleSource; }; - TestFixture(SetupParams params = { }); + TestFixture(SetupParams&& params = { }); struct V8Environment { v8::Isolate* isolate; From 7e8396459d3bcefbbb91031226d92fb4d22d2e11 Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Thu, 26 Oct 2023 17:04:59 -0400 Subject: [PATCH 3/5] Export EmptyReadOnlyActorStorageImpl in server.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m going to want to use this to make an Actor stub in a test. --- src/workerd/server/server.c++ | 47 ----------------------------------- src/workerd/server/server.h | 47 +++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index 2017026d337..fb5abc1bef7 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -115,53 +115,6 @@ static kj::Vector escapeJsonString(kj::StringPtr text) { return escaped; } -// An ActorStorage implementation which will always respond to reads as if the state is empty, -// and will fail any writes. -class EmptyReadOnlyActorStorageImpl final: public rpc::ActorStorage::Stage::Server { -public: - kj::Promise get(GetContext context) override { - return kj::READY_NOW; - } - kj::Promise getMultiple(GetMultipleContext context) override { - return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) - .send().ignoreResult(); - } - kj::Promise list(ListContext context) override { - return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) - .send().ignoreResult(); - } - kj::Promise getAlarm(GetAlarmContext context) override { - return kj::READY_NOW; - } - kj::Promise txn(TxnContext context) override { - auto results = context.getResults(capnp::MessageSize {2, 1}); - results.setTransaction(kj::heap()); - return kj::READY_NOW; - } - -private: - class TransactionImpl final: public rpc::ActorStorage::Stage::Transaction::Server { - protected: - kj::Promise get(GetContext context) override { - return kj::READY_NOW; - } - kj::Promise getMultiple(GetMultipleContext context) override { - return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) - .send().ignoreResult(); - } - kj::Promise list(ListContext context) override { - return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) - .send().ignoreResult(); - } - kj::Promise getAlarm(GetAlarmContext context) override { - return kj::READY_NOW; - } - kj::Promise commit(CommitContext context) override { - return kj::READY_NOW; - } - }; -}; - } // namespace // ======================================================================================= diff --git a/src/workerd/server/server.h b/src/workerd/server/server.h index ec43d945334..4d9dfc97879 100644 --- a/src/workerd/server/server.h +++ b/src/workerd/server/server.h @@ -205,4 +205,51 @@ class Server: private kj::TaskSet::ErrorHandler { kj::ForkedPromise& forkedDrainWhen); }; +// An ActorStorage implementation which will always respond to reads as if the state is empty, +// and will fail any writes. +class EmptyReadOnlyActorStorageImpl final: public rpc::ActorStorage::Stage::Server { +public: + kj::Promise get(GetContext context) override { + return kj::READY_NOW; + } + kj::Promise getMultiple(GetMultipleContext context) override { + return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) + .send().ignoreResult(); + } + kj::Promise list(ListContext context) override { + return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) + .send().ignoreResult(); + } + kj::Promise getAlarm(GetAlarmContext context) override { + return kj::READY_NOW; + } + kj::Promise txn(TxnContext context) override { + auto results = context.getResults(capnp::MessageSize {2, 1}); + results.setTransaction(kj::heap()); + return kj::READY_NOW; + } + +private: + class TransactionImpl final: public rpc::ActorStorage::Stage::Transaction::Server { + protected: + kj::Promise get(GetContext context) override { + return kj::READY_NOW; + } + kj::Promise getMultiple(GetMultipleContext context) override { + return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) + .send().ignoreResult(); + } + kj::Promise list(ListContext context) override { + return context.getParams().getStream().endRequest(capnp::MessageSize {2, 0}) + .send().ignoreResult(); + } + kj::Promise getAlarm(GetAlarmContext context) override { + return kj::READY_NOW; + } + kj::Promise commit(CommitContext context) override { + return kj::READY_NOW; + } + }; +}; + } // namespace workerd::server From 3a33f04b4b57e86c542fd11587cb2ff9953c0930 Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Thu, 26 Oct 2023 14:42:36 -0400 Subject: [PATCH 4/5] Add option to create Actors with the TestFixture I need to test something that requires IoContexts that have Actors associated with them. The easiest way to do this was to extend the TestFixture to create a stubbed out Actor for us. --- src/workerd/tests/test-fixture.c++ | 37 ++++++++++++++++++++++++++++-- src/workerd/tests/test-fixture.h | 3 +++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index 2e3253ee420..b27ee7bea19 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -11,6 +12,7 @@ #include #include #include +#include #include #include @@ -230,6 +232,18 @@ struct MockResponse final: public kj::HttpService::Response { KJ_FAIL_REQUIRE("NOT SUPPORTED"); } }; + +class MockActorLoopback : public Worker::Actor::Loopback, public kj::Refcounted { + public: + virtual kj::Own getWorker(IoChannelFactory::SubrequestMetadata metadata) { + return kj::Own(); + }; + + virtual kj::Own addRef() { + return kj::addRef(*this); + }; +}; + } // namespace @@ -275,7 +289,26 @@ TestFixture::TestFixture(SetupParams&& params) )), errorHandler(kj::heap()), waitUntilTasks(*errorHandler), - headerTable(headerTableBuilder.build()) { } + headerTable(headerTableBuilder.build()) { + KJ_IF_SOME(id, params.actorId) { + auto lock = Worker::Lock(*worker, Worker::Lock::TakeSynchronously(kj::none)); + auto makeActorCache = []( + const ActorCache::SharedLru& sharedLru, OutputGate& outputGate, ActorCache::Hooks& hooks) { + return kj::heap( + kj::heap(), sharedLru, outputGate, hooks); + }; + auto makeStorage = []( + jsg::Lock& js, const Worker::ApiIsolate& apiIsolate, ActorCacheInterface& actorCache) + -> jsg::Ref { + return jsg::alloc( + IoContext::current().addObject(actorCache)); + }; + actor = kj::refcounted( + *worker, /*tracker=*/kj::none, kj::mv(id), /*hasTransient=*/false, makeActorCache, + /*classname=*/kj::none, makeStorage, lock, kj::refcounted(), + *timerChannel, kj::refcounted(), kj::none, kj::none); + } +} void TestFixture::runInIoContext( kj::Function(const Environment&)>&& callback, @@ -310,7 +343,7 @@ void TestFixture::runInIoContext( kj::Own TestFixture::createIncomingRequest() { auto context = kj::refcounted( - threadContext, kj::atomicAddRef(*worker), nullptr, kj::heap()); + threadContext, kj::atomicAddRef(*worker), actor, kj::heap()); auto incomingRequest = kj::heap( kj::addRef(*context), kj::heap(*timerChannel), kj::refcounted(), nullptr); diff --git a/src/workerd/tests/test-fixture.h b/src/workerd/tests/test-fixture.h index 7ad094e1765..f8ed3fe36a4 100644 --- a/src/workerd/tests/test-fixture.h +++ b/src/workerd/tests/test-fixture.h @@ -23,6 +23,8 @@ struct TestFixture { kj::Maybe waitScope; kj::Maybe featureFlags; kj::Maybe mainModuleSource; + // If set, make a stub of an Actor with the given id. + kj::Maybe actorId; }; TestFixture(SetupParams&& params = { }); @@ -88,6 +90,7 @@ struct TestFixture { kj::Own timer; kj::Own timerChannel; kj::Own entropySource; + kj::Maybe> actor; capnp::ByteStreamFactory byteStreamFactory; kj::HttpHeaderTable::Builder headerTableBuilder; ThreadContext::HeaderIdBundle threadContextHeaderBundle; From 2bdbe17c6230c0c5ea7781ae412ceb323b6e00d9 Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Thu, 26 Oct 2023 15:51:06 -0400 Subject: [PATCH 5/5] Log actor id when V8 deserialization fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When V8 deserialization fails, it’s likely due to corruption somewhere between the Actor in JS and the storage layer. In order to aid debugging, log the actor id if we know of one. --- src/workerd/api/BUILD.bazel | 9 ++ .../api/actor-state-iocontext-test.c++ | 96 +++++++++++++++++++ src/workerd/api/actor-state.c++ | 22 ++++- 3 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/workerd/api/actor-state-iocontext-test.c++ diff --git a/src/workerd/api/BUILD.bazel b/src/workerd/api/BUILD.bazel index c3d995592b5..4d42593d5da 100644 --- a/src/workerd/api/BUILD.bazel +++ b/src/workerd/api/BUILD.bazel @@ -101,11 +101,20 @@ wd_cc_capnp_library( ["**/*-test.c++"], exclude = [ "api-rtti-test.c++", + "actor-state-iocontext-test.c++", "cf-property-test.c++", "node/*-test.c++", ], )] +kj_test( + src = "actor-state-iocontext-test.c++", + deps = [ + "//src/workerd/io", + "//src/workerd/tests:test-fixture", + ] +) + kj_test( src = "node/buffer-test.c++", deps = ["//src/workerd/tests:test-fixture"], diff --git a/src/workerd/api/actor-state-iocontext-test.c++ b/src/workerd/api/actor-state-iocontext-test.c++ new file mode 100644 index 00000000000..f69e87daf36 --- /dev/null +++ b/src/workerd/api/actor-state-iocontext-test.c++ @@ -0,0 +1,96 @@ +// Copyright (c) 2017-2023 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +#include + +#include +#include + +#include +#include + +namespace workerd::api { +namespace { + +using workerd::TestFixture; + +bool contains(kj::StringPtr haystack, kj::StringPtr needle) { + return std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end()) + != haystack.end(); +} + +class MockActorId : public ActorIdFactory::ActorId { +public: + MockActorId(kj::String id) : id(kj::mv(id)) {} + kj::String toString() const override { + return kj::str("MockActorId<", id, ">"); + } + + kj::Maybe getName() const override { + return kj::none; + } + + bool equals(const ActorId& other) const override { + return false; + } + + kj::Own clone() const override { + return kj::heap(kj::heapString(id)); + } + + virtual ~MockActorId() {}; +private: + kj::String id; +}; + +void runBadDeserialization(jsg::Lock& lock, kj::StringPtr expectedId) { + // FF = kVersion token, 0E = version 15, 06 = an unknown tag value + kj::StringPtr invalidV8Hex = "FF0E06"_kj; + auto invalidV8Value = kj::decodeHex(invalidV8Hex.asArray()); + try { + deserializeV8Value(lock, "some-key"_kj, invalidV8Value); + KJ_FAIL_ASSERT("deserializeV8Value should have failed."); + } catch (kj::Exception& ex) { + if (ex.getDescription().startsWith("actor storage deserialization failed")) { + KJ_ASSERT(contains(ex.getDescription(), expectedId)); + } else { + throw; + } + } +} + +void runBadDeserializationInIoContext(TestFixture& fixture, kj::StringPtr expectedId) { + fixture.runInIoContext( + [expectedId](const workerd::TestFixture::Environment& env) -> kj::Promise { + runBadDeserialization(env.lock, expectedId); + return kj::READY_NOW; + }); +} + +// TODO(maybe) It would be nice to have a test that tests the case when there's no IoContext, +// but that's a royal pain to set up in this test file we'd basically only test that we don't +// crash, which the actor-state-test.c++ does for us. + +KJ_TEST("no actor specified") { + TestFixture fixture; + runBadDeserializationInIoContext(fixture, "actorId = ;"_kj); +} + +KJ_TEST("actor specified with string id") { + Worker::Actor::Id id = kj::str("testActorId"); + TestFixture fixture(TestFixture::SetupParams{.actorId = kj::mv(id)}); + runBadDeserializationInIoContext(fixture, "actorId = testActorId;"_kj); +} + +KJ_TEST("actor specified with ActorId object") { + kj::Own mockActorId = kj::heap(kj::str("testActorId")); + Worker::Actor::Id id = kj::mv(mockActorId); + TestFixture fixture(TestFixture::SetupParams{ + .actorId = kj::mv(id), + }); + runBadDeserializationInIoContext(fixture, "actorId = MockActorId;"_kj); +} + +} // namespace +} // namespace workerd::api diff --git a/src/workerd/api/actor-state.c++ b/src/workerd/api/actor-state.c++ index 0c4e0964718..58c95af9b97 100644 --- a/src/workerd/api/actor-state.c++ +++ b/src/workerd/api/actor-state.c++ @@ -210,6 +210,25 @@ kj::Promise updateStorageDeletes(IoContext& context, metrics.addStorageDeletes(deleted); }; +// Return the id of the current actor (or the empty string if there is no current actor). +kj::Maybe getCurrentActorId() { + if (IoContext::hasCurrent()) { + IoContext& ioContext = IoContext::current(); + KJ_IF_SOME(actor, ioContext.getActor()) { + KJ_SWITCH_ONEOF(actor.getId()) { + KJ_CASE_ONEOF(s, kj::String) { + return kj::heapString(s); + } + KJ_CASE_ONEOF(actorId, kj::Own) { + return actorId->toString(); + } + } + KJ_UNREACHABLE; + } + } + return kj::none; +} + } // namespace jsg::Promise> DurableObjectStorageOperations::get( @@ -959,9 +978,10 @@ jsg::JsValue deserializeV8Value(jsg::Lock& js, // include the key (to help find the data in the database if it hasn't been deleted), the // length of the value, and the first three bytes of the value (which is just the v8-internal // version header and the tag that indicates the type of the value, but not its contents). + kj::String actorId = getCurrentActorId().orDefault([]() { return kj::str(); }); KJ_FAIL_ASSERT("actor storage deserialization failed", "failed to deserialize stored value", - exception.getHandle(js), key, buf.size(), + actorId, exception.getHandle(js), key, buf.size(), buf.slice(0, std::min(static_cast(3), buf.size()))); }); } catch (jsg::JsExceptionThrown&) {