From 570d259c2934be670dfced8c5059c638535aa66e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 16 Aug 2023 12:57:40 -0700 Subject: [PATCH] Use JsValue in AbortSignal implementation --- src/workerd/api/basics.c++ | 26 +++++++++++++------------- src/workerd/api/basics.h | 23 ++++++++++------------- src/workerd/api/streams/internal.c++ | 4 ++-- src/workerd/api/streams/standard.c++ | 6 +++--- src/workerd/jsg/jsg.h | 6 ++++++ src/workerd/jsg/util.c++ | 20 ++++++++++++++++++++ 6 files changed, 54 insertions(+), 31 deletions(-) diff --git a/src/workerd/api/basics.c++ b/src/workerd/api/basics.c++ index efa5544c6a95..78093b90e9ab 100644 --- a/src/workerd/api/basics.c++ +++ b/src/workerd/api/basics.c++ @@ -361,9 +361,9 @@ bool EventTarget::dispatchEvent(jsg::Lock& js, jsg::Ref event) { kj::Exception AbortSignal::abortException( jsg::Lock& js, - jsg::Optional> maybeReason) { + jsg::Optional maybeReason) { KJ_IF_MAYBE(reason, maybeReason) { - return js.exceptionToKj(js.v8Ref(*reason)); + return js.exceptionToKj(*reason); } return JSG_KJ_EXCEPTION(DISCONNECTED, DOMAbortError, "The operation was aborted"); @@ -371,20 +371,20 @@ kj::Exception AbortSignal::abortException( jsg::Ref AbortSignal::abort( jsg::Lock& js, - jsg::Optional> maybeReason) { + jsg::Optional maybeReason) { auto exception = abortException(js, maybeReason); KJ_IF_MAYBE(reason, maybeReason) { - return jsg::alloc(kj::mv(exception), js.v8Ref(*reason)); + return jsg::alloc(kj::mv(exception), reason->addRef(js)); } - return jsg::alloc(kj::cp(exception), js.exceptionToJs(kj::mv(exception))); + return jsg::alloc(kj::cp(exception), js.exceptionToJsValue(kj::mv(exception))); } void AbortSignal::throwIfAborted(jsg::Lock& js) { if (canceler->isCanceled()) { KJ_IF_MAYBE(r, reason) { - js.throwException(r->addRef(js)); + js.throwException(r->getHandle(js)); } else { - js.throwException(js.exceptionToJs(abortException(js))); + js.throwException(abortException(js)); } } } @@ -402,7 +402,7 @@ jsg::Ref AbortSignal::timeout(jsg::Lock& js, double delay) { // completes, whichever comes first. global.setTimeoutInternal([signal = signal.addRef()](jsg::Lock& js) mutable { - auto exception = js.exceptionToJs(JSG_KJ_EXCEPTION(FAILED, + auto exception = js.exceptionToJsValue(JSG_KJ_EXCEPTION(FAILED, DOMTimeoutError, "The operation was aborted due to timeout")); signal->triggerAbort(js, exception.getHandle(js)); @@ -472,16 +472,16 @@ RefcountedCanceler& AbortSignal::getCanceler() { void AbortSignal::triggerAbort( jsg::Lock& js, - jsg::Optional> maybeReason) { + jsg::Optional maybeReason) { KJ_ASSERT(flag != Flag::NEVER_ABORTS); if (canceler->isCanceled()) { return; } auto exception = AbortSignal::abortException(js, maybeReason); KJ_IF_MAYBE(r, maybeReason) { - reason = js.v8Ref(*r); + reason = r->addRef(js); } else { - reason = js.exceptionToJs(kj::mv(exception)); + reason = js.exceptionToJsValue(kj::mv(exception)); } canceler->cancel(kj::cp(exception)); @@ -498,7 +498,7 @@ void AbortSignal::triggerAbort( void AbortController::abort( jsg::Lock& js, - jsg::Optional> maybeReason) { + jsg::Optional maybeReason) { signal->triggerAbort(js, maybeReason); } @@ -524,7 +524,7 @@ kj::Promise Scheduler::wait( KJ_IF_MAYBE(options, maybeOptions) { KJ_IF_MAYBE(s, options->signal) { if ((*s)->getAborted()) { - return js.exceptionToKj(js.v8Ref((*s)->getReason(js))); + return js.exceptionToKj((*s)->getReason(js)); } } } diff --git a/src/workerd/api/basics.h b/src/workerd/api/basics.h index 578744f576c3..5c25c91f85ab 100644 --- a/src/workerd/api/basics.h +++ b/src/workerd/api/basics.h @@ -466,15 +466,12 @@ class AbortSignal final: public EventTarget { enum class Flag { NONE, NEVER_ABORTS }; AbortSignal(kj::Maybe exception = nullptr, - jsg::Optional> maybeReason = nullptr, + jsg::Optional> maybeReason = nullptr, Flag flag = Flag::NONE) : canceler(IoContext::current().addObject( kj::refcounted(kj::cp(exception)))), - flag(flag) { - KJ_IF_MAYBE(r, maybeReason) { - reason = kj::mv(*r); - } - } + flag(flag), + reason(kj::mv(maybeReason)) {} // The AbortSignal explicitly does not expose a constructor(). It is // illegal for user code to create an AbortSignal directly. @@ -482,11 +479,11 @@ class AbortSignal final: public EventTarget { bool getAborted() { return canceler->isCanceled(); } - v8::Local getReason(jsg::Lock& js) { + jsg::JsValue getReason(jsg::Lock& js) { KJ_IF_MAYBE(r, reason) { return r->getHandle(js); } - return js.v8Undefined(); + return js.undefined(); } void throwIfAborted(jsg::Lock& js); @@ -496,7 +493,7 @@ class AbortSignal final: public EventTarget { static jsg::Ref abort( jsg::Lock& js, - jsg::Optional> reason); + jsg::Optional reason); // The static abort() function here returns an AbortSignal that // has been pre-emptively aborted. It's useful when it might still // be desirable to kick off an async process while communicating @@ -505,7 +502,7 @@ class AbortSignal final: public EventTarget { static jsg::Ref timeout(jsg::Lock& js, double delay); // Returns an AbortSignal that is triggered after delay milliseconds. - void triggerAbort(jsg::Lock& js, jsg::Optional> maybeReason); + void triggerAbort(jsg::Lock& js, jsg::Optional maybeReason); static jsg::Ref any( jsg::Lock& js, @@ -547,14 +544,14 @@ class AbortSignal final: public EventTarget { static kj::Exception abortException( jsg::Lock& js, - jsg::Optional> reason = nullptr); + jsg::Optional reason = nullptr); RefcountedCanceler& getCanceler(); private: IoOwn canceler; Flag flag; - kj::Maybe> reason; + kj::Maybe> reason; void visitForGc(jsg::GcVisitor& visitor); @@ -572,7 +569,7 @@ class AbortController final: public jsg::Object { jsg::Ref getSignal() { return signal.addRef(); } - void abort(jsg::Lock& js, jsg::Optional> reason); + void abort(jsg::Lock& js, jsg::Optional reason); JSG_RESOURCE_TYPE(AbortController, CompatibilityFlags::Reader flags) { if (flags.getJsgPropertyOnPrototypeTemplate()) { diff --git a/src/workerd/api/streams/internal.c++ b/src/workerd/api/streams/internal.c++ index 5d25d0e63a2c..0299d20678fa 100644 --- a/src/workerd/api/streams/internal.c++ +++ b/src/workerd/api/streams/internal.c++ @@ -1479,7 +1479,7 @@ bool WritableStreamInternalController::Pipe::checkSignal(jsg::Lock& js) { auto reason = (*signal)->getReason(js); if (!preventAbort) { KJ_IF_MAYBE(writable, parent.state.tryGet()) { - parent.state.get()->abort(js.exceptionToKj(js.v8Ref(reason))); + parent.state.get()->abort(js.exceptionToKj(reason)); parent.drain(js, reason); } else { parent.writeState.init(); @@ -1488,7 +1488,7 @@ bool WritableStreamInternalController::Pipe::checkSignal(jsg::Lock& js) { parent.writeState.init(); } if (!preventCancel) { - source.release(js, reason); + source.release(js, v8::Local(reason)); } else { source.release(js); } diff --git a/src/workerd/api/streams/standard.c++ b/src/workerd/api/streams/standard.c++ index 0267d11be7bc..d55272d38d75 100644 --- a/src/workerd/api/streams/standard.c++ +++ b/src/workerd/api/streams/standard.c++ @@ -419,13 +419,13 @@ kj::Maybe> WritableLockImpl::PipeLocked::checkSig if ((*signal)->getAborted()) { auto reason = (*signal)->getReason(js); if (!preventCancel) { - source.release(js, reason); + source.release(js, v8::Local(reason)); } else { source.release(js); } if (!preventAbort) { return self.abort(js, reason).then(js, - JSG_VISITABLE_LAMBDA((this, reason = js.v8Ref(reason), ref = self.addRef()), + JSG_VISITABLE_LAMBDA((this, reason = reason.addRef(js), ref = self.addRef()), (reason, ref), (jsg::Lock& js) { return rejectedMaybeHandledPromise( js, @@ -1071,7 +1071,7 @@ jsg::Promise WritableImpl::abort( jsg::Lock& js, jsg::Ref self, v8::Local reason) { - signal->triggerAbort(js, reason); + signal->triggerAbort(js, jsg::JsValue(reason)); // We have to check this again after the AbortSignal is triggered. if (state.template is() || state.template is()) { diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index a96975e5c7af..f82bb9e575dc 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1870,6 +1870,10 @@ class Lock { // as an internal error: the KJ exception message is logged to stderr, and a JavaScript error // is returned with a generic description. + JsRef exceptionToJsValue(kj::Exception&& exception); + + kj::Exception exceptionToKj(const JsValue& exception); + kj::Exception exceptionToKj(Value&& exception); // Encodes the given JavaScript exception into a KJ exception, formatting the description in // such a way that hopefully exceptionToJs() can reproduce something equivalent to the original @@ -1884,6 +1888,8 @@ class Lock { throwException(exceptionToJs(kj::mv(exception))); } + [[noreturn]] void throwException(const JsValue& exception); + template auto tryCatch(Func&& func, ErrorHandler&& errorHandler) -> decltype(func()) { // Invokes `func()` synchronously, catching exceptions. In the event of an exception, diff --git a/src/workerd/jsg/util.c++ b/src/workerd/jsg/util.c++ index 19886d42f2e6..a58ad999f703 100644 --- a/src/workerd/jsg/util.c++ +++ b/src/workerd/jsg/util.c++ @@ -300,6 +300,13 @@ Value Lock::exceptionToJs(kj::Exception&& exception) { }); } +JsRef Lock::exceptionToJsValue(kj::Exception&& exception) { + return withinHandleScope([&] { + JsValue val = JsValue(makeInternalError(v8Isolate, kj::mv(exception))); + return val.addRef(*this); + }); +} + void Lock::throwException(Value&& exception) { withinHandleScope([&] { v8Isolate->ThrowException(exception.getHandle(*this)); @@ -307,6 +314,13 @@ void Lock::throwException(Value&& exception) { throw JsExceptionThrown(); } +void Lock::throwException(const JsValue& exception) { + withinHandleScope([&] { + v8Isolate->ThrowException(exception); + }); + throw JsExceptionThrown(); +} + void throwInternalError(v8::Isolate* isolate, kj::StringPtr internalMessage) { isolate->ThrowException(makeInternalError(isolate, internalMessage)); } @@ -467,6 +481,12 @@ kj::Exception Lock::exceptionToKj(Value&& exception) { }); } +kj::Exception Lock::exceptionToKj(const JsValue& exception) { + return withinHandleScope([&] { + return createTunneledException(v8Isolate, exception); + }); +} + static kj::byte DUMMY = 0; static kj::Array getEmptyArray() { // An older version of asBytes(), when given an empty ArrayBuffer, would often return an array