WIP

nodejs · Nov 6, 2024 · 44787d4 · 44787d4
1 parent 2767456
commit 44787d4
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 50 deletions.
diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js
@@ -1,5 +1,6 @@
 'use strict'
 
+const assert = require('node:assert')
 const DecoratorHandler = require('../handler/decorator-handler')
 
 /**
@@ -19,14 +20,15 @@ const DecoratorHandler = require('../handler/decorator-handler')
 class CacheRevalidationHandler extends DecoratorHandler {
   #successful = false
   /**
-   * @type {((boolean) => void)}
+   * @type {((boolean) => void) | null}
    */
   #callback
   /**
    * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandlers)}
    */
   #handler
 
+  #abort
   /**
    * @param {(boolean) => void} callback Function to call if the cached value is valid
    * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
@@ -44,11 +46,7 @@ class CacheRevalidationHandler extends DecoratorHandler {
 
   onConnect (abort) {
     this.#successful = false
-
-    // TODO (fix): This is suspect...
-    if (typeof this.#handler.onConnect === 'function') {
-      this.#handler.onConnect(abort)
-    }
+    this.#abort = abort
   }
 
   /**
@@ -66,12 +64,21 @@ class CacheRevalidationHandler extends DecoratorHandler {
     resume,
     statusMessage
   ) {
+    assert(this.#callback != null)
+
     // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-a-validation-respo
-    if (statusCode === 304) {
-      this.#successful = true
+    this.#successful = statusCode === 304
+    this.#callback(this.#successful)
+    this.#callback = null
+
+    if (this.#successful) {
       return true
     }
 
+    if (typeof this.#handler.onConnect === 'function') {
+      this.#handler.onConnect(this.#abort)
+    }
+
     if (typeof this.#handler.onHeaders === 'function') {
       return this.#handler.onHeaders(
         statusCode,
@@ -108,9 +115,11 @@ class CacheRevalidationHandler extends DecoratorHandler {
    * @param {string[] | null} rawTrailers
    */
   onComplete (rawTrailers) {
-    this.#callback(this.#successful)
+    if (this.#successful) {
+      return
+    }
 
-    if (!this.#successful && typeof this.#handler.onComplete === 'function') {
+    if (typeof this.#handler.onComplete === 'function') {
       this.#handler.onComplete(rawTrailers)
     }
   }
@@ -121,10 +130,19 @@ class CacheRevalidationHandler extends DecoratorHandler {
    * @param {Error} err
    */
   onError (err) {
-    this.#callback(false)
+    if (this.#successful) {
+      return
+    }
+
+    if (this.#callback) {
+      this.#callback(false)
+      this.#callback = null
+    }
 
     if (typeof this.#handler.onError === 'function') {
       this.#handler.onError(err)
+    } else {
+      throw err
     }
   }
 }

diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js
@@ -39,6 +39,9 @@ module.exports = (opts = {}) => {
 
   return dispatch => {
     return (opts, handler) => {
+      // TODO (fix): What if e.g. opts.headers has if-modified-since header? Or other headers
+      // that make things ambigious?
+
       if (!opts.origin || safeMethodsToNotCache.includes(opts.method)) {
         // Not a method we want to cache or we don't have the origin, skip
         return dispatch(opts, handler)
@@ -66,7 +69,9 @@ module.exports = (opts = {}) => {
                 if (typeof handler.onError === 'function') {
                   handler.onError(err)
                 } else {
-                  throw err
+                  process.nextTick(() => {
+                    throw err
+                  })
                 }
               }
             })
@@ -122,47 +127,39 @@ module.exports = (opts = {}) => {
           return dispatch(opts, new CacheHandler(globalOpts, opts, handler))
         }
 
-        try {
-          // Check if the response is stale
-          const now = Date.now()
-          if (now < value.staleAt) {
-            // Dump body.
-            if (util.isStream(opts.body)) {
-              opts.body.on('error', () => {}).resume()
-            }
-            respondWithCachedValue(stream, value)
-          } else if (util.isStream(opts.body) && util.bodyLength(opts.body) !== 0) {
-            // If body is is stream we can't revalidate...
-            // TODO (fix): This could be less strict...
-            dispatch(opts, new CacheHandler(globalOpts, opts, handler))
-          } else {
-            // Need to revalidate the response
-            dispatch(
-              {
-                ...opts,
-                headers: {
-                  ...opts.headers,
-                  'if-modified-since': new Date(value.cachedAt).toUTCString()
+        // Check if the response is stale
+        const now = Date.now()
+        if (now < value.staleAt) {
+          // Dump request body.
+          if (util.isStream(opts.body)) {
+            opts.body.on('error', () => {}).destroy()
+          }
+          respondWithCachedValue(stream, value)
+        } else if (util.isStream(opts.body) && util.bodyLength(opts.body) !== 0) {
+          // If body is is stream we can't revalidate...
+          // TODO (fix): This could be less strict...
+          dispatch(opts, new CacheHandler(globalOpts, opts, handler))
+        } else {
+          // Need to revalidate the response
+          dispatch(
+            {
+              ...opts,
+              headers: {
+                ...opts.headers,
+                'if-modified-since': new Date(value.cachedAt).toUTCString()
+              }
+            },
+            new CacheRevalidationHandler(
+              (success) => {
+                if (success) {
+                  respondWithCachedValue(stream, value)
+                } else {
+                  stream.on('error', () => {}).destroy()
                 }
               },
-              new CacheRevalidationHandler(
-                (success) => {
-                  if (success) {
-                    respondWithCachedValue(stream, value)
-                  } else {
-                    stream.on('error', () => {}).destroy()
-                  }
-                },
-                new CacheHandler(globalOpts, opts, handler)
-              )
+              new CacheHandler(globalOpts, opts, handler)
             )
-          }
-        } catch (err) {
-          if (typeof handler.onError === 'function') {
-            handler.onError(err)
-          } else {
-            throw err
-          }
+          )
         }
       }