Re-merge Request.signal PR

src/workerd/api/BUILD.bazel

@@ -708,3 +708,9 @@ 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

@@ -521,7 +521,7 @@ class AbortTriggerRpcClient;
 
 class AbortSignal final: public EventTarget {
  public:
-  enum class Flag { NONE, NEVER_ABORTS };
+  enum class Flag { NONE, NEVER_ABORTS, IGNORE_FOR_SUBREQUESTS };
 
   AbortSignal(kj::Maybe<kj::Exception> exception = kj::none,
       jsg::Optional<jsg::JsRef<jsg::JsValue>> maybeReason = kj::none,
@@ -634,9 +634,18 @@ class AbortSignal final: public EventTarget {
 
   JSG_SERIALIZABLE(rpc::SerializationTag::ABORT_SIGNAL);
 
+  // True if this is a signal on the request of an incoming fetch. When the compat flag
+  // `requestSignalPassthrough` is set, this flag has no effect. But to ensure backwards
+  // compatibility, when this flag is not set, this signal will not be passed through to
+  // subrequests derived from the incoming request.
+  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;
 
@@ -681,7 +690,10 @@ class AbortSignal final: public EventTarget {
 // An implementation of the Web Platform Standard AbortController API
 class AbortController final: public jsg::Object {
  public:
-  explicit AbortController(jsg::Lock& js): signal(js.alloc<AbortSignal>()) {}
+  explicit AbortController(
+      jsg::Lock& js, AbortSignal::Flag abortSignalFlag = AbortSignal::Flag::NONE)
+      : signal(js.alloc<AbortSignal>(
+            kj::none /* exception */, kj::none /* maybeReason */, abortSignalFlag)) {}
 
   static jsg::Ref<AbortController> constructor(jsg::Lock& js) {
     return js.alloc<AbortController>(js);

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

@@ -145,7 +145,8 @@ 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<ExportedHandler&> exportedHandler,
+    kj::Maybe<jsg::Ref<AbortSignal>> abortSignal) {
   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
@@ -215,8 +216,18 @@ kj::Promise<DeferredProxy<void>> ServiceWorkerGlobalScope::request(kj::HttpMetho
   }
 
   auto jsRequest = js.alloc<Request>(js, method, url, Request::Redirect::MANUAL, kj::mv(jsHeaders),
-      js.alloc<Fetcher>(IoContext::NEXT_CLIENT_CHANNEL, Fetcher::RequiresHostAndProtocol::YES),
-      kj::none /** AbortSignal **/, kj::mv(cf), kj::mv(body));
+      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
+  // --------
   // 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

@@ -460,7 +460,8 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {
       kj::HttpService::Response& response,
       kj::Maybe<kj::StringPtr> cfBlobJson,
       Worker::Lock& lock,
-      kj::Maybe<ExportedHandler&> exportedHandler);
+      kj::Maybe<ExportedHandler&> exportedHandler,
+      kj::Maybe<jsg::Ref<AbortSignal>> abortSignal);
   // 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++

@@ -1004,7 +1004,7 @@ jsg::Ref<Request> Request::constructor(
       cacheMode = oldRequest->getCacheMode();
       redirect = oldRequest->getRedirectEnum();
       fetcher = oldRequest->getFetcher();
-      signal = oldRequest->getSignal();
+      signal = oldRequest->getThisSignal(js);
     }
   }
 
@@ -1060,6 +1060,7 @@ 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) {
@@ -1113,7 +1114,7 @@ jsg::Ref<Request> Request::constructor(
         cacheMode = otherRequest->cacheMode;
         responseBodyEncoding = otherRequest->responseBodyEncoding;
         fetcher = otherRequest->getFetcher();
-        signal = otherRequest->getSignal();
+        signal = otherRequest->getThisSignal(js);
         headers = js.alloc<Headers>(js, *otherRequest->headers);
         cf = otherRequest->cf.deepClone(js);
         KJ_IF_SOME(b, otherRequest->getBody()) {
@@ -1132,7 +1133,8 @@ jsg::Ref<Request> Request::constructor(
 
   // TODO(conform): If `init` has a keepalive flag, pass it to the Body constructor.
   return js.alloc<Request>(js, method, url, redirect, KJ_ASSERT_NONNULL(kj::mv(headers)),
-      kj::mv(fetcher), kj::mv(signal), kj::mv(cf), kj::mv(body), cacheMode, responseBodyEncoding);
+      kj::mv(fetcher), kj::mv(signal), kj::mv(cf), kj::mv(body), /* thisSignal */ kj::none,
+      cacheMode, responseBodyEncoding);
 }
 
 jsg::Ref<Request> Request::clone(jsg::Lock& js) {
@@ -1142,8 +1144,8 @@ jsg::Ref<Request> Request::clone(jsg::Lock& js) {
   auto bodyClone = Body::clone(js);
 
   return js.alloc<Request>(js, method, url, redirect, kj::mv(headersClone), getFetcher(),
-      /* signal */ getThisSignal(js), kj::mv(cfClone), kj::mv(bodyClone), cacheMode,
-      responseBodyEncoding);
+      /* signal */ getThisSignal(js), kj::mv(cfClone), kj::mv(bodyClone), /* thisSignal */ kj::none,
+      cacheMode, responseBodyEncoding);
 
   // signal
   //-------
@@ -1204,6 +1206,14 @@ jsg::Ref<AbortSignal> Request::getThisSignal(jsg::Lock& js) {
   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;
@@ -2237,6 +2247,11 @@ 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

@@ -793,7 +793,7 @@ class Request final: public Body {
   Request(jsg::Lock& js, 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<Body::ExtractedBody> body, kj::Maybe<jsg::Ref<AbortSignal>> thisSignal,
           CacheMode cacheMode = CacheMode::NONE,
           Response_BodyEncoding responseBodyEncoding = Response_BodyEncoding::AUTO)
     : Body(js, kj::mv(body), *headers), method(method), url(kj::str(url)),
@@ -804,11 +804,15 @@ 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 = kj::mv(s);
+        this->thisSignal = s.addRef();
       } else {
-        this->signal = kj::mv(s);
+        this->signal = s.addRef();
       }
     }
+
+    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
@@ -860,9 +864,13 @@ 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(jsg::Lock& js);
 
+  // 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();
+
   // Returns the `cf` field containing Cloudflare feature flags.
   jsg::Optional<jsg::JsObject> getCf(jsg::Lock& js);