From 17d7a3c39fdb0a8d3cb500bf48bf59ee2999af7f Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 10:37:11 -0700 Subject: [PATCH 1/6] Explicitly ignore promises in actor-cache-test.c++. Sometimes the test wants to wait on the result of expectUncached(), and sometimes it doesn't. When it doesn't, cast the result to void to suppress warnings from discarding a promise. I did try waiting on some of these promises instead of dropping them, but that causes test failures. Seems like the dropping is intentional. --- src/workerd/io/actor-cache-test.c++ | 100 ++++++++++++++-------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/workerd/io/actor-cache-test.c++ b/src/workerd/io/actor-cache-test.c++ index c60ce6285c7..e964be4c20e 100644 --- a/src/workerd/io/actor-cache-test.c++ +++ b/src/workerd/io/actor-cache-test.c++ @@ -974,9 +974,9 @@ KJ_TEST("ActorCache canceled deletes are coalesced") { auto& mockStorage = test.mockStorage; // A bunch of deletes where we immediately drop the returned promises. - expectUncached(test.delete_("foo")); - expectUncached(test.delete_({"bar"_kj, "baz"_kj})); - expectUncached(test.delete_("qux")); + (void)expectUncached(test.delete_("foo")); + (void)expectUncached(test.delete_({"bar"_kj, "baz"_kj})); + (void)expectUncached(test.delete_("qux")); // Keep one promise. auto promise = expectUncached(test.delete_("corge")); @@ -1530,9 +1530,9 @@ KJ_TEST("ActorCache get-multiple multiple blocks") { // At this point, "bar" and "baz" are considered cached. KJ_ASSERT(expectCached(test.get("bar")) == nullptr); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("baz"))) == "456"); - expectUncached(test.get("corge")); - expectUncached(test.get("foo")); - expectUncached(test.get("qux")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("foo")); + (void)expectUncached(test.get("qux")); stream.call("values", CAPNP(list = [(key = "foo", value = "789")])) .expectReturns(CAPNP(), ws); @@ -1542,7 +1542,7 @@ KJ_TEST("ActorCache get-multiple multiple blocks") { KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("baz"))) == "456"); KJ_ASSERT(expectCached(test.get("corge")) == nullptr); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("foo"))) == "789"); - expectUncached(test.get("qux")); + (void)expectUncached(test.get("qux")); stream.call("end", CAPNP()).expectReturns(CAPNP(), ws); @@ -1712,7 +1712,7 @@ KJ_TEST("ActorCache list() with limit") { KJ_ASSERT(expectCached(test.get("fon")) == nullptr); // Stuff after the last key is not in cache. - expectUncached(test.get("fooa")); + (void)expectUncached(test.get("fooa")); // Listing the same range again, with the same limit or lower, is fully cached. KJ_ASSERT(expectCached(test.list("bar", "qux", 3)) == @@ -1859,9 +1859,9 @@ KJ_TEST("ActorCache list() multiple ranges") { KJ_ASSERT(expectCached(test.list("a", "c")) == kvs({{"a", "1"}, {"b", "2"}})); KJ_ASSERT(expectCached(test.list("x", "z")) == kvs({{"y", "9"}})); - expectUncached(test.get("w")); - expectUncached(test.get("d")); - expectUncached(test.get("c")); + (void)expectUncached(test.get("w")); + (void)expectUncached(test.get("d")); + (void)expectUncached(test.get("c")); } KJ_TEST("ActorCache list() with some already-cached keys in range") { @@ -2714,7 +2714,7 @@ KJ_TEST("ActorCache listReverse() with limit") { KJ_ASSERT(expectCached(test.get("fon")) == nullptr); // Stuff before the first key is not in cache. - expectUncached(test.get("baq")); + (void)expectUncached(test.get("baq")); // Listing the same range again, with the same limit or lower, is fully cached. KJ_ASSERT(expectCached(test.listReverse("bar", "qux", 3)) == @@ -2861,9 +2861,9 @@ KJ_TEST("ActorCache listReverse() multiple ranges") { KJ_ASSERT(expectCached(test.listReverse("a", "c")) == kvs({{"b", "2"}, {"a", "1"}})); KJ_ASSERT(expectCached(test.listReverse("x", "z")) == kvs({{"y", "9"}})); - expectUncached(test.get("w")); - expectUncached(test.get("d")); - expectUncached(test.get("c")); + (void)expectUncached(test.get("w")); + (void)expectUncached(test.get("d")); + (void)expectUncached(test.get("c")); } KJ_TEST("ActorCache listReverse() with some already-cached keys in range") { @@ -3524,7 +3524,7 @@ KJ_TEST("ActorCache LRU purge") { KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("bar"))) == "456"); // But foo was evicted. - expectUncached(test.get("foo")); + (void)expectUncached(test.get("foo")); } KJ_TEST("ActorCache LRU purge ordering") { @@ -3555,8 +3555,8 @@ KJ_TEST("ActorCache LRU purge ordering") { // Foo and qux live, bar and baz evicted. KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("foo"))) == "123"); - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("qux"))) == "555"); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("xxx"))) == "aaa"); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("yyy"))) == "bbb"); @@ -3615,9 +3615,9 @@ KJ_TEST("ActorCache LRU purge larger") { test.gate.wait().wait(ws); } - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); - expectUncached(test.get("qux")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); + (void)expectUncached(test.get("qux")); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("corge"))) == kilobyte); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("grault"))) == kilobyte); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("garply"))) == kilobyte); @@ -3643,9 +3643,9 @@ KJ_TEST("ActorCache LRU purge") { // Nothing was cached, because nothing fit in the LRU. KJ_ASSERT(test.lru.currentSize() == 0); - expectUncached(test.get("foo")); - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); + (void)expectUncached(test.get("foo")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); } KJ_TEST("ActorCache evict on timeout") { @@ -3687,7 +3687,7 @@ KJ_TEST("ActorCache evict on timeout") { // Now foo should be evicted and bar and baz stale. // Verify foo is evicted. - expectUncached(test.get("foo")); + (void)expectUncached(test.get("foo")); // Touch bar. expectCached(test.get("bar")); @@ -3696,7 +3696,7 @@ KJ_TEST("ActorCache evict on timeout") { // Now baz should have been evicted, but bar is still here because we keep touching it. expectCached(test.get("bar")); - expectUncached(test.get("baz")); + (void)expectUncached(test.get("baz")); } KJ_TEST("ActorCache backpressure due to dirtyPressureThreshold") { @@ -3801,9 +3801,9 @@ KJ_TEST("ActorCache lru evict entry with known-empty gaps") { KJ_ASSERT(expectCached(test.list("foo", "qux")) == kvs({{"foo", "123"}})); KJ_ASSERT(expectCached(test.get("fooa")) == nullptr); - expectUncached(test.get("baza")); - expectUncached(test.get("corge")); - expectUncached(test.get("fo")); + (void)expectUncached(test.get("baza")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("fo")); } KJ_TEST("ActorCache lru evict gap entry with known-empty gaps") { @@ -3852,7 +3852,7 @@ KJ_TEST("ActorCache lru evict gap entry with known-empty gaps") { } // Okay, that gap is gone now. - expectUncached(test.get("foo+1")); + (void)expectUncached(test.get("foo+1")); } KJ_TEST("ActorCache lru evict entry with trailing known-empty gap (followed by END_GAP)") { @@ -3900,11 +3900,11 @@ KJ_TEST("ActorCache lru evict entry with trailing known-empty gap (followed by E KJ_ASSERT(expectCached(test.list("bar", "corge")) == kvs({{"bar", "456"}, {"baz", "789"}})); KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("corge"))) == "555"); - expectUncached(test.get("corgf")); - expectUncached(test.get("foo")); - expectUncached(test.get("quw")); - expectUncached(test.get("qux")); - expectUncached(test.get("quy")); + (void)expectUncached(test.get("corgf")); + (void)expectUncached(test.get("foo")); + (void)expectUncached(test.get("quw")); + (void)expectUncached(test.get("qux")); + (void)expectUncached(test.get("quy")); } KJ_TEST("ActorCache timeout entry with known-empty gaps") { @@ -3955,9 +3955,9 @@ KJ_TEST("ActorCache timeout entry with known-empty gaps") { KJ_ASSERT(expectCached(test.list("foo", "qux")) == kvs({{"foo", "123"}})); KJ_ASSERT(expectCached(test.get("fooa")) == nullptr); - expectUncached(test.get("baza")); - expectUncached(test.get("corge")); - expectUncached(test.get("fo")); + (void)expectUncached(test.get("baza")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("fo")); } @@ -4025,10 +4025,10 @@ KJ_TEST("ActorCache purge everything while listing") { kvs({{"bar", "456"}, {"baz", "789"}, {"corge", "555"}, {"foo", "123"}})); } - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); - expectUncached(test.get("corge")); - expectUncached(test.get("foo")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("foo")); } KJ_TEST("ActorCache purge everything while listing; has previous entry") { @@ -4229,12 +4229,12 @@ KJ_TEST("ActorCache skip cache") { KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("foo"))) == "qux"); // The other things that were returned weren't cached. - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); // No gaps were cached as empty either. - expectUncached(test.get("corge")); - expectUncached(test.get("grault")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("grault")); // Again, but reverse list. { @@ -4257,12 +4257,12 @@ KJ_TEST("ActorCache skip cache") { KJ_ASSERT(KJ_ASSERT_NONNULL(expectCached(test.get("foo"))) == "qux"); // The other things that were returned weren't cached. - expectUncached(test.get("bar")); - expectUncached(test.get("baz")); + (void)expectUncached(test.get("bar")); + (void)expectUncached(test.get("baz")); // No gaps were cached as empty either. - expectUncached(test.get("corge")); - expectUncached(test.get("grault")); + (void)expectUncached(test.get("corge")); + (void)expectUncached(test.get("grault")); } // ======================================================================================= From 6f4a22761bdf360db3361a338a4ff952978f9f20 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 10:43:00 -0700 Subject: [PATCH 2/6] Fix dropped promises in io-gate-test.c++ --- src/workerd/io/io-gate-test.c++ | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workerd/io/io-gate-test.c++ b/src/workerd/io/io-gate-test.c++ index f4f1ecbaa18..c7648f323fe 100644 --- a/src/workerd/io/io-gate-test.c++ +++ b/src/workerd/io/io-gate-test.c++ @@ -135,7 +135,7 @@ KJ_TEST("InputGate nested critical sections") { } // Start cs2. - cs2->wait(); + cs2->wait().wait(ws); // Can't start new tasks in cs1 until cs2 finishes. auto cs1Wait = cs1->wait(); @@ -166,7 +166,7 @@ KJ_TEST("InputGate nested critical section outlives parent") { } // Start cs2. - cs2->wait(); + cs2->wait().wait(ws); // Mark cs1 done. (Note that, in a real program, this probably can't happen like this, because a // lock would be taken on cs1 before marking it done, and that lock would wait for cs2 to @@ -210,7 +210,7 @@ KJ_TEST("InputGate deeply nested critical sections") { } // Start cs2 - cs2->wait(); + cs2->wait().wait(ws); // Add some waiters to cs2, some of which are waiting to start more nested critical sections auto lock = cs2->wait().wait(ws); @@ -336,7 +336,7 @@ KJ_TEST("InputGate broken") { } // start cs2 - cs2->wait(); + cs2->wait().wait(ws); auto cs1Wait = cs1->wait(); KJ_EXPECT(!cs1Wait.poll(ws)); From dcdc001b53b67310afdca02b775eeb5e8db50292 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 10:47:43 -0700 Subject: [PATCH 3/6] Fix dropped promise in websocket hibernation This looks like a simple missing "co_await". Earlier in the method, we have "co_await ws.receive()", so I think it's okay to "co_await ws.send(...)" as well. --- src/workerd/io/hibernation-manager.c++ | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workerd/io/hibernation-manager.c++ b/src/workerd/io/hibernation-manager.c++ index cc8da03f44b..e2662a0d1bc 100644 --- a/src/workerd/io/hibernation-manager.c++ +++ b/src/workerd/io/hibernation-manager.c++ @@ -207,7 +207,7 @@ kj::Promise HibernationManagerImpl::readLoop(HibernatableWebSocket& hib) { // autoResponseTimestamp on the active websocket. (active)->setAutoResponseTimestamp(hib.autoResponseTimestamp); } - ws.send((reqResp)->getResponse().asArray()); + co_await ws.send((reqResp)->getResponse().asArray()); skip = true; // If we've sent an auto response message, we should not unhibernate or deliver the // received message to the actor From 83dab2621e26cd92fd93e6d99bdcac86c59cc284 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 10:59:16 -0700 Subject: [PATCH 4/6] Fix dropped drain promises Instead of awaiting sequentially, I opted to drain everything concurrently for lower latency. --- src/workerd/server/server.c++ | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index 8e861a3b182..5064845f8d4 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -2546,9 +2546,11 @@ kj::Promise Server::handleDrain(kj::Promise drainWhen) { co_await drainWhen; // Tell all HttpServers to drain. This causes them to disconnect any connections that don't // have a request in-flight. + auto drainPromises = kj::heapArrayBuilder>(httpServers.size()); for (auto& httpServer: httpServers) { - httpServer.httpServer.drain(); + drainPromises.add(httpServer.httpServer.drain()); } + co_await kj::joinPromisesFailFast(drainPromises.finish()); } kj::Promise Server::run(jsg::V8System& v8System, config::Config::Reader config, From 2e238eef28546dbca34ebceb48f3f15036b6ab10 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 11:08:42 -0700 Subject: [PATCH 5/6] Fix (sort of) dropped promises in DigestStreamSink::write. There's no actual bug here. DigestStreamSink::write doesn't do any IO (it's a cryptographic digest; it just computes things), but it has to return kj::Promise since it implements WritableStreamSink. This just silences a compiler warning. --- src/workerd/api/crypto.c++ | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/workerd/api/crypto.c++ b/src/workerd/api/crypto.c++ index 450faf62d30..b8115db4821 100644 --- a/src/workerd/api/crypto.c++ +++ b/src/workerd/api/crypto.c++ @@ -700,9 +700,8 @@ kj::Promise DigestStreamSink::write(const void* buffer, size_t size) { kj::Promise DigestStreamSink::write(kj::ArrayPtr> pieces) { for (auto& piece : pieces) { - write(piece.begin(), piece.size()); + co_await write(piece.begin(), piece.size()); } - return kj::READY_NOW; } kj::Promise DigestStreamSink::end() { From 351c2f12bd74e1bcae8350b612f23ce57c5e4b1e Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 21 Sep 2023 11:20:21 -0700 Subject: [PATCH 6/6] Fix dropped promise in ServiceWorkerGlobalScope::request. --- src/workerd/api/global-scope.c++ | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/workerd/api/global-scope.c++ b/src/workerd/api/global-scope.c++ index ef2b3230802..c439badeca2 100644 --- a/src/workerd/api/global-scope.c++ +++ b/src/workerd/api/global-scope.c++ @@ -218,8 +218,8 @@ kj::Promise> ServiceWorkerGlobalScope::request( if (jsStream->isDisturbed()) { lock.logUncaughtException( "Script consumed request body but didn't call respondWith(). Can't forward request."); - response.sendError(500, "Internal Server Error", ioContext.getHeaderTable()); - return addNoopDeferredProxy(kj::READY_NOW); + return addNoopDeferredProxy( + response.sendError(500, "Internal Server Error", ioContext.getHeaderTable())); } else { auto client = ioContext.getHttpClient( IoContext::NEXT_CLIENT_CHANNEL, false,