Merge pull request #3735 from cloudflare/mar/request-signal

Revert request Request.signal detection to client disconnects.

Author: mar-cf

src/workerd/api/BUILD.bazel

@@ -680,9 +680,3 @@ wd_test(
     args = ["--experimental"],
     data = ["tests/disable-importable-env-test.js"],
 )
-
-wd_test(
-    src = "tests/request-client-disconnect.wd-test",
-    args = ["--experimental"],
-    data = ["tests/request-client-disconnect.js"],
-)

src/workerd/api/basics.h

@@ -512,7 +512,7 @@ class EventTarget: public jsg::Object {
 // An implementation of the Web Platform Standard AbortSignal API
 class AbortSignal final: public EventTarget {
  public:
-  enum class Flag { NONE, NEVER_ABORTS, IGNORE_FOR_SUBREQUESTS };
+  enum class Flag { NONE, NEVER_ABORTS };
 
   AbortSignal(kj::Maybe<kj::Exception> exception = kj::none,
       jsg::Optional<jsg::JsRef<jsg::JsValue>> maybeReason = kj::none,
@@ -603,14 +603,9 @@ class AbortSignal final: public EventTarget {
     tracker.trackField("reason", reason);
   }
 
-  bool isIgnoredForSubrequests() const {
-    return flag == Flag::IGNORE_FOR_SUBREQUESTS;
-  }
-
  private:
   IoOwn<RefcountedCanceler> canceler;
   Flag flag;
-
   kj::Maybe<jsg::JsRef<jsg::JsValue>> reason;
   kj::Maybe<jsg::JsRef<jsg::JsValue>> onAbortHandler;
 
@@ -622,9 +617,7 @@ class AbortSignal final: public EventTarget {
 // An implementation of the Web Platform Standard AbortController API
 class AbortController final: public jsg::Object {
  public:
-  explicit AbortController(AbortSignal::Flag abortSignalFlag = AbortSignal::Flag::NONE)
-      : signal(jsg::alloc<AbortSignal>(
-            kj::none /* exception */, kj::none /* maybeReason */, abortSignalFlag)) {}
+  explicit AbortController(): signal(jsg::alloc<AbortSignal>()) {}
 
   static jsg::Ref<AbortController> constructor() {
     return jsg::alloc<AbortController>();

src/workerd/api/global-scope.c++

@@ -145,8 +145,7 @@ kj::Promise<DeferredProxy<void>> ServiceWorkerGlobalScope::request(kj::HttpMetho
     kj::HttpService::Response& response,
     kj::Maybe<kj::StringPtr> cfBlobJson,
     Worker::Lock& lock,
-    kj::Maybe<ExportedHandler&> exportedHandler,
-    kj::Maybe<jsg::Ref<AbortSignal>> abortSignal) {
+    kj::Maybe<ExportedHandler&> exportedHandler) {
   TRACE_EVENT("workerd", "ServiceWorkerGlobalScope::request()");
   // To construct a ReadableStream object, we're supposed to pass in an Own<AsyncInputStream>, so
   // that it can drop the reference whenever it gets GC'ed. But in this case the stream's lifetime
@@ -217,17 +216,7 @@ kj::Promise<DeferredProxy<void>> ServiceWorkerGlobalScope::request(kj::HttpMetho
 
   auto jsRequest = jsg::alloc<Request>(method, url, Request::Redirect::MANUAL, kj::mv(jsHeaders),
       jsg::alloc<Fetcher>(IoContext::NEXT_CLIENT_CHANNEL, Fetcher::RequiresHostAndProtocol::YES),
-      /* signal */ kj::none, kj::mv(cf), kj::mv(body),
-      /* thisSignal */ kj::mv(abortSignal), Request::CacheMode::NONE);
-
-  // signal vs thisSignal
-  // --------------------
-  // The fetch spec definition of Request has a distinction between the
-  // "signal" (which is an optional AbortSignal passed in with the options), and "this' signal",
-  // which is an AbortSignal that is always available via the request.signal accessor.
-  //
-  // redirect
-  // --------
+      kj::none /** AbortSignal **/, kj::mv(cf), kj::mv(body));
   // I set the redirect mode to manual here, so that by default scripts that just pass requests
   // through to a fetch() call will behave the same as scripts which don't call .respondWith(): if
   // the request results in a redirect, the visitor will see that redirect.

src/workerd/api/global-scope.h

@@ -463,8 +463,7 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {
       kj::HttpService::Response& response,
       kj::Maybe<kj::StringPtr> cfBlobJson,
       Worker::Lock& lock,
-      kj::Maybe<ExportedHandler&> exportedHandler,
-      kj::Maybe<jsg::Ref<AbortSignal>> abortSignal);
+      kj::Maybe<ExportedHandler&> exportedHandler);
   // TODO(cleanup): Factor out the shared code used between old-style event listeners vs. module
   //   exports and move that code somewhere more appropriate.
 

src/workerd/api/http.c++

@@ -996,7 +996,7 @@ jsg::Ref<Request> Request::constructor(
       cacheMode = oldRequest->getCacheMode();
       redirect = oldRequest->getRedirectEnum();
       fetcher = oldRequest->getFetcher();
-      signal = oldRequest->getThisSignal();
+      signal = oldRequest->getSignal();
     }
   }
 
@@ -1052,7 +1052,6 @@ jsg::Ref<Request> Request::constructor(
           // explicitly say `signal: null`, they must want to drop the signal that was on the
           // original request.
           signal = kj::mv(s);
-          initDict.signal = kj::none;
         }
 
         KJ_IF_SOME(newCf, initDict.cf) {
@@ -1106,7 +1105,7 @@ jsg::Ref<Request> Request::constructor(
         cacheMode = otherRequest->cacheMode;
         responseBodyEncoding = otherRequest->responseBodyEncoding;
         fetcher = otherRequest->getFetcher();
-        signal = otherRequest->getThisSignal();
+        signal = otherRequest->getSignal();
         headers = jsg::alloc<Headers>(*otherRequest->headers);
         cf = otherRequest->cf.deepClone(js);
         KJ_IF_SOME(b, otherRequest->getBody()) {
@@ -1125,8 +1124,7 @@ jsg::Ref<Request> Request::constructor(
 
   // TODO(conform): If `init` has a keepalive flag, pass it to the Body constructor.
   return jsg::alloc<Request>(method, url, redirect, KJ_ASSERT_NONNULL(kj::mv(headers)),
-      kj::mv(fetcher), kj::mv(signal), kj::mv(cf), kj::mv(body), /* thisSignal */ kj::none,
-      cacheMode, responseBodyEncoding);
+      kj::mv(fetcher), kj::mv(signal), kj::mv(cf), kj::mv(body), cacheMode, responseBodyEncoding);
 }
 
 jsg::Ref<Request> Request::clone(jsg::Lock& js) {
@@ -1136,8 +1134,8 @@ jsg::Ref<Request> Request::clone(jsg::Lock& js) {
   auto bodyClone = Body::clone(js);
 
   return jsg::alloc<Request>(method, url, redirect, kj::mv(headersClone), getFetcher(),
-      /* signal */ getThisSignal(), kj::mv(cfClone), kj::mv(bodyClone), /* thisSignal */ kj::none,
-      cacheMode, responseBodyEncoding);
+      /* signal */ getThisSignal(js), kj::mv(cfClone), kj::mv(bodyClone), cacheMode,
+      responseBodyEncoding);
 
   // signal
   //-------
@@ -1186,7 +1184,7 @@ jsg::Optional<jsg::JsObject> Request::getCf(jsg::Lock& js) {
 // that's a bit silly and unnecessary.
 // The name "thisSignal" is derived from the fetch spec, which draws a
 // distinction between the "signal" and "this' signal".
-jsg::Ref<AbortSignal> Request::getThisSignal() {
+jsg::Ref<AbortSignal> Request::getThisSignal(jsg::Lock& js) {
   KJ_IF_SOME(s, signal) {
     return s.addRef();
   }
@@ -1198,14 +1196,6 @@ jsg::Ref<AbortSignal> Request::getThisSignal() {
   return newSignal;
 }
 
-void Request::clearSignalIfIgnoredForSubrequest() {
-  KJ_IF_SOME(s, signal) {
-    if (s->isIgnoredForSubrequests()) {
-      signal = kj::none;
-    }
-  }
-}
-
 kj::Maybe<Request::Redirect> Request::tryParseRedirect(kj::StringPtr redirect) {
   if (strcasecmp(redirect.cStr(), "follow") == 0) {
     return Redirect::FOLLOW;
@@ -2257,11 +2247,6 @@ jsg::Promise<jsg::Ref<Response>> fetchImplNoOutputLock(jsg::Lock& js,
     // front is robust, and won't add significant overhead compared to the rest of fetch().
     auto jsRequest = Request::constructor(js, kj::mv(requestOrUrl), kj::mv(requestInit));
 
-    // Clear the request's signal if the 'ignoreForSubrequests' flag is set. This happens when
-    // a request from an incoming fetch is passed-through to another fetch. We want to avoid
-    // aborting the subrequest in that case.
-    jsRequest->clearSignalIfIgnoredForSubrequest();
-
     // This URL list keeps track of redirections and becomes a source for Response's URL list. The
     // first URL in the list is the Request's URL (visible to JS via Request::getUrl()). The last URL
     // in the list is the Request's "current" URL (eventually visible to JS via Response::getUrl()).

src/workerd/api/http.h

@@ -791,7 +791,7 @@ class Request final: public Body {
   Request(kj::HttpMethod method, kj::StringPtr url, Redirect redirect,
           jsg::Ref<Headers> headers, kj::Maybe<jsg::Ref<Fetcher>> fetcher,
           kj::Maybe<jsg::Ref<AbortSignal>> signal, CfProperty&& cf,
-          kj::Maybe<Body::ExtractedBody> body, kj::Maybe<jsg::Ref<AbortSignal>> thisSignal,
+          kj::Maybe<Body::ExtractedBody> body,
           CacheMode cacheMode = CacheMode::NONE,
           Response_BodyEncoding responseBodyEncoding = Response_BodyEncoding::AUTO)
     : Body(kj::mv(body), *headers), method(method), url(kj::str(url)),
@@ -802,15 +802,11 @@ class Request final: public Body {
       // that the cancel machinery is not used but the request.signal accessor will still
       // do the right thing.
       if (s->getNeverAborts()) {
-        this->thisSignal = s.addRef();
+        this->thisSignal = kj::mv(s);
       } else {
-        this->signal = s.addRef();
+        this->signal = kj::mv(s);
       }
     }
-
-    KJ_IF_SOME(s, thisSignal) {
-      this->thisSignal = s.addRef();
-    }
   }
   // TODO(conform): Technically, the request's URL should be parsed immediately upon Request
   //   construction, and any errors encountered should be thrown. Instead, we defer parsing until
@@ -862,12 +858,8 @@ class Request final: public Body {
   // request.signal to always return an AbortSignal even if one is not actively
   // used on this request.
   kj::Maybe<jsg::Ref<AbortSignal>> getSignal();
-  jsg::Ref<AbortSignal> getThisSignal();
 
-  // Clear the request's signal if the 'ignoreForSubrequests' flag is set. This happens when
-  // a request from an incoming fetch is passed-through to another fetch. We want to avoid
-  // aborting the subrequest in that case.
-  void clearSignalIfIgnoredForSubrequest();
+  jsg::Ref<AbortSignal> getThisSignal(jsg::Lock& js);
 
   // Returns the `cf` field containing Cloudflare feature flags.
   jsg::Optional<jsg::JsObject> getCf(jsg::Lock& js);