From 923ab9f62c5b198d0eeb994f649a4718270c3a98 Mon Sep 17 00:00:00 2001 From: Felix Hanau Date: Tue, 23 Dec 2025 13:50:38 -0500 Subject: [PATCH] [o11y] Do error checking in R2 earlier Before #5790, this would have avoided "destructed WorkerTracer" warnings when we would construct a WorkerInterface but not end up using it due to the errors being detected later. However, this 1) does not apply for prod where R2 is implemented differently and 2) no longer fixes a warning since the WorkerInterface never has delivered() called. However, it is still good practice to check for errors earlier and avoid memory allocations from the getHttpClient() calls. While this PR is not aiming for completion I also cleaned up a call in web-socket.c++ that may be susceptible to the same issue. --- src/workerd/api/r2-bucket.c++ | 12 ++++++------ src/workerd/api/r2-multipart.c++ | 6 +++--- src/workerd/api/web-socket.c++ | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/workerd/api/r2-bucket.c++ b/src/workerd/api/r2-bucket.c++ index c0f71eb12f7..f2e0a0d4e82 100644 --- a/src/workerd/api/r2-bucket.c++ +++ b/src/workerd/api/r2-bucket.c++ @@ -452,7 +452,6 @@ jsg::Promise>> R2Bucket::head(jsg::Lock auto traceSpan = context.makeTraceSpan("r2_head"_kjc); auto userSpan = context.makeUserTraceSpan("r2_head"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -478,6 +477,7 @@ jsg::Promise>> R2Bucket::head(jsg::Lock auto requestJson = json.encode(requestBuilder); kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPGetRequest(kj::mv(client), kj::mv(requestJson), path, jwt, flags); return context.awaitIo(js, kj::mv(promise), @@ -509,7 +509,6 @@ R2Bucket::get(jsg::Lock& js, auto traceSpan = context.makeTraceSpan("r2_get"_kjc); auto userSpan = context.makeUserTraceSpan("r2_get"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -538,6 +537,7 @@ R2Bucket::get(jsg::Lock& js, auto requestJson = json.encode(requestBuilder); kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPGetRequest(kj::mv(client), kj::mv(requestJson), path, jwt, flags); return context.awaitIo(js, kj::mv(promise), @@ -599,7 +599,6 @@ jsg::Promise>> R2Bucket::put(jsg::Lock& auto traceSpan = context.makeTraceSpan("r2_put"_kjc); auto userSpan = context.makeUserTraceSpan("r2_put"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -841,6 +840,7 @@ jsg::Promise>> R2Bucket::put(jsg::Lock& cancelReader.cancel(); kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest(kj::mv(client), kj::mv(value), kj::none, kj::mv(requestJson), path, jwt); @@ -875,7 +875,6 @@ jsg::Promise> R2Bucket::createMultipartUpload(jsg::L auto traceSpan = context.makeTraceSpan("r2_createMultipartUpload"_kjc); auto userSpan = context.makeUserTraceSpan("r2_createMultipartUpload"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -967,6 +966,7 @@ jsg::Promise> R2Bucket::createMultipartUpload(jsg::L auto requestJson = json.encode(requestBuilder); kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest(kj::mv(client), kj::none, kj::none, kj::mv(requestJson), path, jwt); @@ -1005,7 +1005,6 @@ jsg::Promise R2Bucket::delete_(jsg::Lock& js, auto traceSpan = context.makeTraceSpan("r2_delete"_kjc); auto userSpan = context.makeUserTraceSpan("r2_delete"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -1042,6 +1041,7 @@ jsg::Promise R2Bucket::delete_(jsg::Lock& js, kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest(kj::mv(client), kj::none, kj::none, kj::mv(requestJson), path, jwt); @@ -1068,7 +1068,6 @@ jsg::Promise R2Bucket::list(jsg::Lock& js, auto traceSpan = context.makeTraceSpan("r2_list"_kjc); auto userSpan = context.makeUserTraceSpan("r2_list"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bindingName()) { @@ -1173,6 +1172,7 @@ jsg::Promise R2Bucket::list(jsg::Lock& js, kj::StringPtr components[1]; auto path = fillR2Path(components, adminBucket); + auto client = context.getHttpClient(clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPGetRequest(kj::mv(client), kj::mv(requestJson), path, jwt, flags); return context.awaitIo(js, kj::mv(promise), diff --git a/src/workerd/api/r2-multipart.c++ b/src/workerd/api/r2-multipart.c++ index f0ec5b36917..b494b20104b 100644 --- a/src/workerd/api/r2-multipart.c++ +++ b/src/workerd/api/r2-multipart.c++ @@ -45,7 +45,6 @@ jsg::Promise R2MultipartUpload::uploadPart(jsg: auto traceSpan = context.makeTraceSpan("r2_uploadPart"_kjc); auto userSpan = context.makeUserTraceSpan("r2_uploadPart"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bucket->bindingName()) { @@ -120,6 +119,7 @@ jsg::Promise R2MultipartUpload::uploadPart(jsg: kj::StringPtr components[1]; auto path = fillR2Path(components, this->bucket->adminBucket); + auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest( kj::mv(client), kj::mv(value), kj::none, kj::mv(requestJson), path, kj::none); @@ -152,7 +152,6 @@ jsg::Promise> R2MultipartUpload::complete(jsg::Lo auto traceSpan = context.makeTraceSpan("r2_completeMultipartUpload"_kjc); auto userSpan = context.makeUserTraceSpan("r2_completeMultipartUpload"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bucket->bindingName()) { @@ -195,6 +194,7 @@ jsg::Promise> R2MultipartUpload::complete(jsg::Lo kj::StringPtr components[1]; auto path = fillR2Path(components, this->bucket->adminBucket); + auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest(kj::mv(client), kj::none, kj::none, kj::mv(requestJson), path, kj::none); @@ -223,7 +223,6 @@ jsg::Promise R2MultipartUpload::abort( auto traceSpan = context.makeTraceSpan("r2_abortMultipartUpload"_kjc); auto userSpan = context.makeUserTraceSpan("r2_abortMultipartUpload"_kjc); TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); - auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); traceContext.userSpan.setTag("cloudflare.binding.type"_kjc, "r2"_kjc); KJ_IF_SOME(b, this->bucket->bindingName()) { @@ -251,6 +250,7 @@ jsg::Promise R2MultipartUpload::abort( kj::StringPtr components[1]; auto path = fillR2Path(components, this->bucket->adminBucket); + auto client = context.getHttpClient(this->bucket->clientIndex, true, kj::none, traceContext); auto promise = doR2HTTPPutRequest(kj::mv(client), kj::none, kj::none, kj::mv(requestJson), path, kj::none); diff --git a/src/workerd/api/web-socket.c++ b/src/workerd/api/web-socket.c++ index 81cfec706b2..17ca17f88f4 100644 --- a/src/workerd/api/web-socket.c++ +++ b/src/workerd/api/web-socket.c++ @@ -210,7 +210,6 @@ jsg::Ref WebSocket::constructor(jsg::Lock& js, urlRecord.fragment == kj::none, DOMSyntaxError, wsErr, "The url fragment must be empty."); kj::HttpHeaders headers(context.getHeaderTable()); - auto client = context.getHttpClient(0, false, kj::none, "websocket_open"_kjc); // Set protocols header if necessary. KJ_IF_SOME(variant, protocols) { @@ -260,6 +259,7 @@ jsg::Ref WebSocket::constructor(jsg::Lock& js, headers.unset(kj::HttpHeaderId::SEC_WEBSOCKET_EXTENSIONS); } + auto client = context.getHttpClient(0, false, kj::none, "websocket_open"_kjc); auto prom = ([](auto& context, auto connUrl, auto headers, auto client) -> kj::Promise { auto response = co_await client->openWebSocket(connUrl, headers);