fix: cache (nodejs#3804)

* fix: cache

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* fixuP

* WIP

* WIP

Author: ronag

lib/cache/memory-cache-store.js

@@ -294,6 +294,7 @@ class MemoryStoreReadableStream extends Readable {
    * @type {MemoryStoreValue}
    */
   #value
+
   /**
    * @type {Buffer[]}
    */

lib/handler/cache-handler.js

@@ -7,6 +7,8 @@ const {
   parseVaryHeader
 } = require('../util/cache')
 
+function noop () {}
+
 /**
  * Writes a response to a CacheStore and then passes it on to the next handler
  */
@@ -46,6 +48,17 @@ class CacheHandler extends DecoratorHandler {
     this.#handler = handler
   }
 
+  onConnect (abort) {
+    if (this.#writeStream) {
+      this.#writeStream.destroy()
+      this.#writeStream = undefined
+    }
+
+    if (typeof this.#handler.onConnect === 'function') {
+      this.#handler.onConnect(abort)
+    }
+  }
+
   /**
    * @see {DispatchHandlers.onHeaders}
    *
@@ -61,49 +74,46 @@ class CacheHandler extends DecoratorHandler {
     resume,
     statusMessage
   ) {
-    const downstreamOnHeaders = () => this.#handler.onHeaders(
-      statusCode,
-      rawHeaders,
-      resume,
-      statusMessage
-    )
+    const downstreamOnHeaders = () => {
+      if (typeof this.#handler.onHeaders === 'function') {
+        return this.#handler.onHeaders(
+          statusCode,
+          rawHeaders,
+          resume,
+          statusMessage
+        )
+      } else {
+        return true
+      }
+    }
 
     if (
       !util.safeHTTPMethods.includes(this.#requestOptions.method) &&
       statusCode >= 200 &&
       statusCode <= 399
     ) {
-      // https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons
-      // Try/catch for if it's synchronous
+      // https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-response
       try {
-        const result = this.#store.deleteByOrigin(this.#requestOptions.origin)
-        if (
-          result &&
-          typeof result.catch === 'function' &&
-          typeof this.#handler.onError === 'function'
-        ) {
-          // Fail silently
-          result.catch(_ => {})
-        }
-      } catch (err) {
+        this.#store.deleteByOrigin(this.#requestOptions.origin.toString())?.catch?.(noop)
+      } catch {
         // Fail silently
       }
-
       return downstreamOnHeaders()
     }
 
     const headers = util.parseHeaders(rawHeaders)
 
     const cacheControlHeader = headers['cache-control']
-    const contentLengthHeader = headers['content-length']
-
-    if (!cacheControlHeader || !contentLengthHeader || this.#store.isFull) {
-      // Don't have the headers we need, can't cache
+    if (!cacheControlHeader || typeof cacheControlHeader !== 'string') {
+      // Don't have cache-control, can't cache.
       return downstreamOnHeaders()
     }
 
-    const contentLength = Number(contentLengthHeader)
+    const contentLengthHeader = headers['content-length']
+    const contentLength = contentLengthHeader ? Number(contentLengthHeader) : null
     if (!Number.isInteger(contentLength)) {
+      // Don't know the final size, don't cache.
+      // TODO (fix): Why not cache?
       return downstreamOnHeaders()
     }
 
@@ -137,18 +147,24 @@ class CacheHandler extends DecoratorHandler {
       })
 
       if (this.#writeStream) {
-        this.#writeStream.on('drain', resume)
-        this.#writeStream.on('error', () => {
-          this.#writeStream = undefined
-          resume()
-        })
+        const handler = this
+        this.#writeStream
+          .on('drain', resume)
+          .on('error', function () {
+            // TODO (fix): Make error somehow observable?
+          })
+          .on('close', function () {
+            if (handler.#writeStream === this) {
+              handler.#writeStream = undefined
+            }
+
+            // TODO (fix): Should we resume even if was paused downstream?
+            resume()
+          })
       }
     }
 
-    if (typeof this.#handler.onHeaders === 'function') {
-      return downstreamOnHeaders()
-    }
-    return false
+    return downstreamOnHeaders()
   }
 
   /**
@@ -178,10 +194,7 @@ class CacheHandler extends DecoratorHandler {
    */
   onComplete (rawTrailers) {
     if (this.#writeStream) {
-      if (rawTrailers) {
-        this.#writeStream.rawTrailers = rawTrailers
-      }
-
+      this.#writeStream.rawTrailers = rawTrailers ?? []
       this.#writeStream.end()
     }
 
@@ -332,7 +345,7 @@ function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirective
   for (let i = 0; i < headerNames.length; i++) {
     const header = headerNames[i]
 
-    if (headersToRemove.indexOf(header) !== -1) {
+    if (headersToRemove.includes(header)) {
       // We have a at least one header we want to remove
       if (!strippedHeaders) {
         // This is the first header we want to remove, let's create the object

lib/handler/cache-revalidation-handler.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const assert = require('node:assert')
 const DecoratorHandler = require('../handler/decorator-handler')
 
 /**
@@ -19,29 +20,35 @@ const DecoratorHandler = require('../handler/decorator-handler')
 class CacheRevalidationHandler extends DecoratorHandler {
   #successful = false
   /**
-   * @type {(() => void)}
+   * @type {((boolean) => void) | null}
    */
-  #successCallback
+  #callback
   /**
    * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandlers)}
    */
   #handler
 
+  #abort
   /**
-   * @param {() => void} successCallback Function to call if the cached value is valid
+   * @param {(boolean) => void} callback Function to call if the cached value is valid
    * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
    */
-  constructor (successCallback, handler) {
-    if (typeof successCallback !== 'function') {
-      throw new TypeError('successCallback must be a function')
+  constructor (callback, handler) {
+    if (typeof callback !== 'function') {
+      throw new TypeError('callback must be a function')
     }
 
     super(handler)
 
-    this.#successCallback = successCallback
+    this.#callback = callback
     this.#handler = handler
   }
 
+  onConnect (abort) {
+    this.#successful = false
+    this.#abort = abort
+  }
+
   /**
    * @see {DispatchHandlers.onHeaders}
    *
@@ -57,13 +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.#successCallback()
+    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,
@@ -72,7 +87,8 @@ class CacheRevalidationHandler extends DecoratorHandler {
         statusMessage
       )
     }
-    return false
+
+    return true
   }
 
   /**
@@ -90,7 +106,7 @@ class CacheRevalidationHandler extends DecoratorHandler {
       return this.#handler.onData(chunk)
     }
 
-    return false
+    return true
   }
 
   /**
@@ -99,7 +115,11 @@ class CacheRevalidationHandler extends DecoratorHandler {
    * @param {string[] | null} rawTrailers
    */
   onComplete (rawTrailers) {
-    if (!this.#successful && typeof this.#handler.onComplete === 'function') {
+    if (this.#successful) {
+      return
+    }
+
+    if (typeof this.#handler.onComplete === 'function') {
       this.#handler.onComplete(rawTrailers)
     }
   }
@@ -110,8 +130,19 @@ class CacheRevalidationHandler extends DecoratorHandler {
    * @param {Error} err
    */
   onError (err) {
+    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
     }
   }
 }