fix: fixes cache leak and unhandled promise rejection (#152)

Fixes some bugs in the new request cache:

1. When stopping the client we were supposed to remove the response cache, instead we were deleting an item from the cache
2. When stopping we were `void`ing the cache deletion - this can cause unhandled promise rejections so switches to `await`ing the promise instead
3. When starting the cache was being created asynchronously but without waiting for the promise to resolve, so you could stop the client before the cache was intialized which would mean it is never cleaned up

Also not a bug but it also makes the cache name configurable.

Author: achingbrain

packages/client/src/client.ts

@@ -21,7 +21,8 @@ const log = logger('delegated-routing-v1-http-api-client')
 const defaultValues = {
   concurrentRequests: 4,
   timeout: 30e3,
-  cacheTTL: 5 * 60 * 1000 // 5 minutes default as per https://specs.ipfs.tech/routing/http-routing-v1/#response-headers
+  cacheTTL: 5 * 60 * 1000, // 5 minutes default as per https://specs.ipfs.tech/routing/http-routing-v1/#response-headers
+  cacheName: 'delegated-routing-v1-cache'
 }
 
 export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpApiClient {
@@ -35,6 +36,7 @@ export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV
   private readonly filterAddrs?: string[]
   private readonly filterProtocols?: string[]
   private readonly inFlightRequests: Map<string, Promise<Response>>
+  private readonly cacheName: string
   private cache?: Cache
   private readonly cacheTTL: number
   /**
@@ -55,17 +57,8 @@ export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV
     this.contentRouting = new DelegatedRoutingV1HttpApiClientContentRouting(this)
     this.peerRouting = new DelegatedRoutingV1HttpApiClientPeerRouting(this)
 
+    this.cacheName = init.cacheName ?? defaultValues.cacheName
     this.cacheTTL = init.cacheTTL ?? defaultValues.cacheTTL
-    const cacheEnabled = (typeof globalThis.caches !== 'undefined') && (this.cacheTTL > 0)
-
-    if (cacheEnabled) {
-      log('cache enabled with ttl %d', this.cacheTTL)
-      globalThis.caches.open('delegated-routing-v1-cache').then(cache => {
-        this.cache = cache
-      }).catch(() => {
-        this.cache = undefined
-      })
-    }
   }
 
   get [contentRoutingSymbol] (): ContentRouting {
@@ -80,19 +73,30 @@ export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV
     return this.started
   }
 
-  start (): void {
+  async start (): Promise<void> {
+    if (this.started) {
+      return
+    }
+
     this.started = true
+
+    if (this.cacheTTL > 0) {
+      this.cache = await globalThis.caches?.open(this.cacheName)
+
+      if (this.cache != null) {
+        log('cache enabled with ttl %d', this.cacheTTL)
+      }
+    }
   }
 
-  stop (): void {
+  async stop (): Promise<void> {
     this.httpQueue.clear()
     this.shutDownController.abort()
-    this.started = false
 
     // Clear the cache when stopping
-    if (this.cache != null) {
-      void this.cache.delete('delegated-routing-v1-cache')
-    }
+    await globalThis.caches?.delete(this.cacheName)
+
+    this.started = false
   }
 
   async * getProviders (cid: CID, options: GetProvidersOptions = {}): AsyncGenerator<PeerRecord> {

packages/client/src/index.ts

@@ -144,6 +144,15 @@ export interface DelegatedRoutingV1HttpApiClientInit extends FilterOptions {
    * If 0, caching is disabled
    */
   cacheTTL?: number
+
+  /**
+   * Where a [Cache](https://developer.mozilla.org/en-US/docs/Web/API/Cache) is
+   * available in the global scope, we will store request/responses to avoid
+   * making duplicate requests.
+   *
+   * @default 'delegated-routing-v1-cache'
+   */
+  cacheName?: string
 }
 
 export interface GetIPNSOptions extends AbortOptions {
@@ -182,11 +191,17 @@ export interface DelegatedRoutingV1HttpApiClient {
    */
   putIPNS(libp2pKey: CID<unknown, 0x72, 0x00 | 0x12, 1>, record: IPNSRecord, options?: AbortOptions): Promise<void>
 
+  /**
+   * Create the request/response cache used to ensure duplicate requests aren't
+   * made for the same data
+   */
+  start(): Promise<void>
+
   /**
    * Shut down any currently running HTTP requests and clear up any resources
    * that are in use
    */
-  stop(): void
+  stop(): Promise<void>
 }
 
 /**

packages/client/test/client.spec.ts

@@ -0,0 +1,17 @@
+import { expect } from 'aegir/chai'
+import { DefaultDelegatedRoutingV1HttpApiClient } from '../src/client.js'
+import { itBrowser } from './fixtures/it.js'
+
+describe('client', () => {
+  itBrowser('should remove cache on stop', async function () {
+    const cacheName = 'test-cache'
+
+    const client = new DefaultDelegatedRoutingV1HttpApiClient('http://example.com', {
+      cacheName
+    })
+    await client.start()
+    await client.stop()
+
+    await expect(globalThis.caches.has(cacheName)).to.eventually.be.false('did not remove cache on stop')
+  })
+})

packages/client/test/fixtures/it.ts

@@ -0,0 +1,5 @@
+/* eslint-env mocha */
+
+import { isBrowser } from 'wherearewe'
+
+export const itBrowser = (isBrowser ? it : it.skip)

packages/client/test/index.spec.ts

@@ -1,33 +1,32 @@
 /* eslint-env mocha */
 
 import { generateKeyPair } from '@libp2p/crypto/keys'
+import { start, stop } from '@libp2p/interface'
 import { peerIdFromPrivateKey, peerIdFromString } from '@libp2p/peer-id'
 import { multiaddr } from '@multiformats/multiaddr'
 import { expect } from 'aegir/chai'
 import { createIPNSRecord, marshalIPNSRecord } from 'ipns'
 import all from 'it-all'
 import { CID } from 'multiformats/cid'
-import { isBrowser } from 'wherearewe'
 import { createDelegatedRoutingV1HttpApiClient, type DelegatedRoutingV1HttpApiClient } from '../src/index.js'
+import { itBrowser } from './fixtures/it.js'
 
 if (process.env.ECHO_SERVER == null) {
   throw new Error('Echo server not configured correctly')
 }
 
 const serverUrl = process.env.ECHO_SERVER
-const itBrowser = (isBrowser ? it : it.skip)
 
 describe('delegated-routing-v1-http-api-client', () => {
   let client: DelegatedRoutingV1HttpApiClient
 
-  beforeEach(() => {
+  beforeEach(async () => {
     client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), { cacheTTL: 0 })
+    await start(client)
   })
 
   afterEach(async () => {
-    if (client != null) {
-      client.stop()
-    }
+    await stop(client)
   })
 
   it('should find providers', async () => {
@@ -357,6 +356,7 @@ describe('delegated-routing-v1-http-api-client', () => {
     const clientWithShortTTL = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), {
       cacheTTL: shortTTL
     })
+    await start(clientWithShortTTL)
 
     const cid = CID.parse('QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn')
     const providers = [{
@@ -395,6 +395,6 @@ describe('delegated-routing-v1-http-api-client', () => {
     callCount = parseInt(await (await fetch(`${process.env.ECHO_SERVER}/get-call-count`)).text(), 10)
     expect(callCount).to.equal(2) // Second server call after cache expired
 
-    clientWithShortTTL.stop()
+    await stop(clientWithShortTTL)
   })
 })

packages/client/test/routings.spec.ts

@@ -2,7 +2,7 @@
 /* eslint-env mocha */
 
 import { generateKeyPair } from '@libp2p/crypto/keys'
-import { contentRoutingSymbol, peerRoutingSymbol } from '@libp2p/interface'
+import { contentRoutingSymbol, peerRoutingSymbol, start, stop } from '@libp2p/interface'
 import { peerIdFromPrivateKey } from '@libp2p/peer-id'
 import { expect } from 'aegir/chai'
 import { createIPNSRecord, marshalIPNSRecord, multihashToIPNSRoutingKey } from 'ipns'
@@ -22,14 +22,13 @@ if (serverUrl == null) {
 describe('libp2p content-routing', () => {
   let client: DelegatedRoutingV1HttpApiClient
 
-  beforeEach(() => {
+  beforeEach(async () => {
     client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), { cacheTTL: 0 })
+    await start(client)
   })
 
   afterEach(async () => {
-    if (client != null) {
-      client.stop()
-    }
+    await stop(client)
 
     const res = await fetch(`${process.env.ECHO_SERVER}/reset-call-count`)
     await res.text()
@@ -192,14 +191,13 @@ describe('libp2p content-routing', () => {
 describe('libp2p peer-routing', () => {
   let client: DelegatedRoutingV1HttpApiClient
 
-  beforeEach(() => {
+  beforeEach(async () => {
     client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl))
+    await start(client)
   })
 
   afterEach(async () => {
-    if (client != null) {
-      client.stop()
-    }
+    await stop(client)
   })
 
   describe('peer routing', () => {

packages/interop/test/index.spec.ts

@@ -4,6 +4,7 @@ import { createDelegatedRoutingV1HttpApiClient } from '@helia/delegated-routing-
 import { createDelegatedRoutingV1HttpApiServer } from '@helia/delegated-routing-v1-http-api-server'
 import { ipns } from '@helia/ipns'
 import { generateKeyPair } from '@libp2p/crypto/keys'
+import { start, stop } from '@libp2p/interface'
 import { expect } from 'aegir/chai'
 import { createIPNSRecord } from 'ipns'
 import first from 'it-first'
@@ -43,12 +44,12 @@ describe('delegated-routing-v1-http-api interop', () => {
         await node.libp2p.dial(remote.libp2p.getMultiaddrs())
       }
     }
+
+    await start(client)
   })
 
   afterEach(async () => {
-    if (client != null) {
-      client.stop()
-    }
+    await stop(client)
 
     if (server != null) {
       await server.close()