-
Notifications
You must be signed in to change notification settings - Fork 421
Fix AbortController creation in global scope #3856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
// Create a global AbortController instance at module scope to verify | ||
// that it's possible to create AbortControllers in the global scope | ||
// This test validates that the fix for https://github.com/cloudflare/workerd/issues/3657 works properly | ||
const globalAbortController = new AbortController(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of issues with this that make this a bit iffy... While this does allow creating the AbortController
at the global scope, it will end up binding it to the first IoContext
that accesses it, so any attempt access it from other requests will fail, and any attempt to access it at the global scope will fail. Because of that I'm really not sure if this change is worthwhile. Interested in @kentonv's thoughts.
12c21a4
to
2ec1f1e
Compare
This change makes AbortSignal lazily initialize its canceler only when needed, allowing AbortController/AbortSignal to be created in global scope without throwing an exception. It will still throw when used for I/O operations in global scope. Fixes #3657
2ec1f1e
to
872065b
Compare
One possible change on top of this is diff --git a/src/workerd/api/basics.c++ b/src/workerd/api/basics.c++
index a56c880b..5fdb058b 100644
--- a/src/workerd/api/basics.c++
+++ b/src/workerd/api/basics.c++
@@ -548,7 +548,7 @@ jsg::Ref<AbortSignal> AbortSignal::abort(jsg::Lock& js, jsg::Optional<jsg::JsVal
}
void AbortSignal::throwIfAborted(jsg::Lock& js) {
- if (getCanceler().isCanceled()) {
+ if (getAborted()) {
KJ_IF_SOME(r, reason) {
js.throwException(r.getHandle(js));
} else {
@@ -645,10 +645,12 @@ RefcountedCanceler& AbortSignal::getCanceler() {
void AbortSignal::triggerAbort(
jsg::Lock& js, jsg::Optional<kj::OneOf<kj::Exception, jsg::JsValue>> maybeReason) {
KJ_ASSERT(flag != Flag::NEVER_ABORTS);
- if (getCanceler().isCanceled()) {
+ if (getAborted()) {
return;
}
auto exception = AbortSignal::abortException(js, maybeReason);
+ pendingException = kj::cp(exception);
+
KJ_IF_SOME(r, maybeReason) {
KJ_SWITCH_ONEOF(r) {
KJ_CASE_ONEOF(value, jsg::JsValue) {
@@ -661,7 +663,11 @@ void AbortSignal::triggerAbort(
} else {
reason = js.exceptionToJsValue(kj::mv(exception));
}
- getCanceler().cancel(kj::cp(exception));
+
+ // If we already have a canceler, cancel it
+ KJ_IF_SOME(c, canceler) {
+ c->cancel(kj::cp(exception));
+ }
// This is questionable only because it goes against the spec but it does help prevent
// memory leaks. Once the abort signal has been triggered, there's really nothing else
diff --git a/src/workerd/api/basics.h b/src/workerd/api/basics.h
index 4c806f7e..f7408380 100644
--- a/src/workerd/api/basics.h
+++ b/src/workerd/api/basics.h
@@ -523,7 +523,10 @@ class AbortSignal final: public EventTarget {
static jsg::Ref<AbortSignal> constructor() = delete;
bool getAborted() {
- return getCanceler().isCanceled();
+ KJ_IF_SOME(c, canceler) {
+ return c->isCanceled();
+ }
+ return pendingException != kj::none;
}
jsg::JsValue getReason(jsg::Lock& js);
@@ -577,8 +580,7 @@ class AbortSignal final: public EventTarget {
// Allows this AbortSignal to also serve as a kj::Canceler
template <typename T>
kj::Promise<T> wrap(kj::Promise<T> promise) {
- JSG_REQUIRE(
- !getCanceler().isCanceled(), TypeError, "The AbortSignal has already been triggered");
+ JSG_REQUIRE(!getAborted(), TypeError, "The AbortSignal has already been triggered");
return getCanceler().wrap(kj::mv(promise));
} |
This change makes AbortSignal check its aborted state without requiring an IoContext, allowing AbortController/AbortSignal to be created in global scope without throwing an exception. It will still require an IoContext when used for I/O operations. Fixes #3657
@@ -631,16 +630,27 @@ void AbortSignal::visitForGc(jsg::GcVisitor& visitor) { | |||
} | |||
|
|||
RefcountedCanceler& AbortSignal::getCanceler() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach just is not going to work. To understand why, consider the following case:
// Global scope
const ac = new AbortController();
export default {
fetch(req) {
ac.signal.throwIfAborted();
}
}
Now let's say you throw two separate requests at this worker. The AbortController
will end up being bound to the first received requests IoContext
via the create IoOwn
. When the subsequent request comes in, it will cause an error because of that internal IoOwn
belonging to a different IoContext
.
What would be a better approach, if we think AbortController
really needs to be supported at the global scope, would be to remove the underlying use of kj::Canceler
in its current form and instead of relying on that, cancel via a registered event listener.
Ultimately I don't think Claudes approach here is at all correct.
Summary
Test plan
bazel test //src/workerd/api:tests/abortsignal-test
Fixes #3657