Skip to content

Commit

Permalink
Include the typename T in the error message when checkFarGet fails (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
jasnell authored Apr 18, 2024
1 parent ec539a3 commit 7af1346
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 19 deletions.
39 changes: 35 additions & 4 deletions src/workerd/api/tests/js-rpc-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -857,18 +857,49 @@ export let crossContextSharingDoesntWork = {
// tryUseGlobalRpcPromise() tries to return the result, it tries to serialize the stub, but
// it can't do that from the wrong context.
globalRpcPromise = env.MyService.makeCounter(12);
await assert.rejects(() => env.MyService.tryUseGlobalRpcPromise(), expectedError);

await assert.rejects(() => env.MyService.tryUseGlobalRpcPromise(), {
name: "Error",
message:
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
"request/response bodies, and others) created in the context of one request handler " +
"cannot be accessed from a different request's handler. This is a limitation of " +
"Cloudflare Workers which allows us to improve overall performance. (I/O type: Client)"
});

// Pipelining on someone else's promise straight-up doesn't work.
await assert.rejects(() => env.MyService.tryUseGlobalRpcPromisePipeline(), expectedError);
await assert.rejects(() => env.MyService.tryUseGlobalRpcPromisePipeline(), {
name: "Error",
message:
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
"request/response bodies, and others) created in the context of one request handler " +
"cannot be accessed from a different request's handler. This is a limitation of " +
"Cloudflare Workers which allows us to improve overall performance."
});

// Now let's try accessing a JsRpcProperty, where the property is NOT a direct property of a
// top-level service binding. This works even less than a JsRpcPromise, since there's no inner
// JS promise, it tries to create one on-demand, which fails because the parent object is
// tied to the original I/O context.
globalRpcPromise = env.MyService.getAnObject(5).counter;
await assert.rejects(() => env.MyService.tryUseGlobalRpcPromise(), expectedError);
await assert.rejects(() => env.MyService.tryUseGlobalRpcPromisePipeline(), expectedError);

await assert.rejects(() => env.MyService.tryUseGlobalRpcPromise(), {
name: "Error",
message:
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
"request/response bodies, and others) created in the context of one request handler " +
"cannot be accessed from a different request's handler. This is a limitation of " +
"Cloudflare Workers which allows us to improve overall performance. (I/O type: Pipeline)"
});

await assert.rejects(() => env.MyService.tryUseGlobalRpcPromisePipeline(), {
name: "Error",
message:
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
"request/response bodies, and others) created in the context of one request handler " +
"cannot be accessed from a different request's handler. This is a limitation of " +
"Cloudflare Workers which allows us to improve overall performance."
});
},
}

Expand Down
18 changes: 11 additions & 7 deletions src/workerd/io/io-context.c++
Original file line number Diff line number Diff line change
Expand Up @@ -956,14 +956,14 @@ void IoContext::requireCurrent() {
KJ_REQUIRE(threadLocalRequest == this, "request is not current in this thread");
}

void IoContext::checkFarGet(const DeleteQueue* expectedQueue) {
void IoContext::checkFarGet(const DeleteQueue* expectedQueue, kj::StringPtr type) {
KJ_ASSERT(expectedQueue);
requireCurrent();

if (expectedQueue == deleteQueue.get()) {
// same request or same actor, success
} else {
throwNotCurrentJsError();
throwNotCurrentJsError(type);
}
}

Expand Down Expand Up @@ -1273,20 +1273,24 @@ void IoContext::requireCurrentOrThrowJs(WeakRef& weak) {
throwNotCurrentJsError();
}

void IoContext::throwNotCurrentJsError() {
void IoContext::throwNotCurrentJsError(kj::Maybe<kj::StringPtr> maybeType) {
auto type = maybeType.map([](kj::StringPtr type) {
return kj::str(" (I/O type: ", type, ")");
}).orDefault(kj::String());

if (threadLocalRequest != nullptr && threadLocalRequest->actor != kj::none) {
JSG_FAIL_REQUIRE(Error,
"Cannot perform I/O on behalf of a different Durable Object. I/O objects "
kj::str("Cannot perform I/O on behalf of a different Durable Object. I/O objects "
"(such as streams, request/response bodies, and others) created in the context of one "
"Durable Object cannot be accessed from a different Durable Object in the same isolate. "
"This is a limitation of Cloudflare Workers which allows us to improve overall "
"performance.");
"performance.", type));
} else {
JSG_FAIL_REQUIRE(Error,
"Cannot perform I/O on behalf of a different request. I/O objects (such as "
kj::str("Cannot perform I/O on behalf of a different request. I/O objects (such as "
"streams, request/response bodies, and others) created in the context of one request "
"handler cannot be accessed from a different request's handler. This is a limitation "
"of Cloudflare Workers which allows us to improve overall performance.");
"of Cloudflare Workers which allows us to improve overall performance.", type));
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/workerd/io/io-context.h
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
static void requireCurrentOrThrowJs(WeakRef& weak);

// Just throw the error that requireCurrentOrThrowJs() would throw on failure.
[[noreturn]] static void throwNotCurrentJsError();
[[noreturn]] static void throwNotCurrentJsError(kj::Maybe<kj::StringPtr> maybeType = kj::none);

// -----------------------------------------------------------------
// Task scheduling and object storage
Expand Down Expand Up @@ -811,7 +811,7 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler

void taskFailed(kj::Exception&& exception) override;
void requireCurrent();
void checkFarGet(const DeleteQueue* expectedQueue);
void checkFarGet(const DeleteQueue* expectedQueue, kj::StringPtr type);

kj::Maybe<jsg::JsRef<jsg::JsObject>> promiseContextTag;

Expand Down
4 changes: 2 additions & 2 deletions src/workerd/io/io-own.c++
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ void DeleteQueue::scheduleDeletion(OwnedObject* object) const {
}
}

void DeleteQueue::checkFarGet(const DeleteQueue* deleteQueue) {
IoContext::current().checkFarGet(deleteQueue);
void DeleteQueue::checkFarGet(const DeleteQueue* deleteQueue, kj::StringPtr type) {
IoContext::current().checkFarGet(deleteQueue, type);
}

OwnedObjectList::~OwnedObjectList() noexcept(false) {
Expand Down
13 changes: 9 additions & 4 deletions src/workerd/io/io-own.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include <kj/refcount.h>
#include <kj/string.h>
#include <kj/vector.h>
#include <typeinfo>
#include <workerd/jsg/util.h>

namespace workerd {

Expand Down Expand Up @@ -118,7 +120,7 @@ class DeleteQueue: public kj::AtomicRefcounted {
// Implements the corresponding methods of IoContext and ActorContext.
template <typename T> IoOwn<T> addObject(kj::Own<T> obj, OwnedObjectList& ownedObjects);

static void checkFarGet(const DeleteQueue* deleteQueue);
static void checkFarGet(const DeleteQueue* deleteQueue, kj::StringPtr type);
};

template <typename T>
Expand Down Expand Up @@ -286,13 +288,15 @@ IoPtr<T>& IoPtr<T>::operator=(decltype(nullptr)) {

template <typename T>
inline T* IoOwn<T>::operator->() {
DeleteQueue::checkFarGet(deleteQueue);
auto type = jsg::typeName(typeid(T));
DeleteQueue::checkFarGet(deleteQueue, type);
return item->ptr;
}

template <typename T>
inline IoOwn<T>::operator kj::Own<T>() && {
DeleteQueue::checkFarGet(deleteQueue);
auto type = jsg::typeName(typeid(T));
DeleteQueue::checkFarGet(deleteQueue, type);
auto result = kj::mv(item->ptr);
OwnedObjectList::unlink(*item);
item = nullptr;
Expand All @@ -302,7 +306,8 @@ inline IoOwn<T>::operator kj::Own<T>() && {

template <typename T>
inline T* IoPtr<T>::operator->() {
DeleteQueue::checkFarGet(deleteQueue);
auto type = jsg::typeName(typeid(T));
DeleteQueue::checkFarGet(deleteQueue, type);
return ptr;
}

Expand Down

0 comments on commit 7af1346

Please sign in to comment.