From 7aa64a03e7b5111ff928359f9a558d4b5c1a14b2 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Mon, 18 Nov 2024 15:27:18 +0000 Subject: [PATCH 1/5] fix: fixes cache leak 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 unhandeled 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 --- packages/client/src/client.ts | 36 ++++++++++++++++------------- packages/client/src/constants.ts | 1 + packages/client/src/index.ts | 9 ++++++++ packages/client/test/client.spec.ts | 17 ++++++++++++++ packages/client/test/fixtures/it.ts | 5 ++++ packages/client/test/index.spec.ts | 5 ++-- 6 files changed, 55 insertions(+), 18 deletions(-) create mode 100644 packages/client/src/constants.ts create mode 100644 packages/client/test/client.spec.ts create mode 100644 packages/client/test/fixtures/it.ts diff --git a/packages/client/src/client.ts b/packages/client/src/client.ts index 5417aa0..ba115d7 100644 --- a/packages/client/src/client.ts +++ b/packages/client/src/client.ts @@ -9,6 +9,7 @@ import { ipnsValidator } from 'ipns/validator' import { parse as ndjson } from 'it-ndjson' import defer from 'p-defer' import PQueue from 'p-queue' +import { CACHE_NAME } from './constants.js' import { BadResponseError, InvalidRequestError } from './errors.js' import { DelegatedRoutingV1HttpApiClientContentRouting, DelegatedRoutingV1HttpApiClientPeerRouting } from './routings.js' import type { DelegatedRoutingV1HttpApiClient, DelegatedRoutingV1HttpApiClientInit, GetProvidersOptions, GetPeersOptions, GetIPNSOptions, PeerRecord } from './index.js' @@ -35,6 +36,7 @@ export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV private readonly filterAddrs?: string[] private readonly filterProtocols?: string[] private readonly inFlightRequests: Map> + 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 ?? CACHE_NAME 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 { + 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 { 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 { diff --git a/packages/client/src/constants.ts b/packages/client/src/constants.ts new file mode 100644 index 0000000..e92e118 --- /dev/null +++ b/packages/client/src/constants.ts @@ -0,0 +1 @@ +export const CACHE_NAME = 'delegated-routing-v1-cache' diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 393db52..41bc501 100644 --- a/packages/client/src/index.ts +++ b/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 { diff --git a/packages/client/test/client.spec.ts b/packages/client/test/client.spec.ts new file mode 100644 index 0000000..4dcd767 --- /dev/null +++ b/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') + }) +}) diff --git a/packages/client/test/fixtures/it.ts b/packages/client/test/fixtures/it.ts new file mode 100644 index 0000000..2ee2670 --- /dev/null +++ b/packages/client/test/fixtures/it.ts @@ -0,0 +1,5 @@ +/* eslint-env mocha */ + +import { isBrowser } from 'wherearewe' + +export const itBrowser = (isBrowser ? it : it.skip) diff --git a/packages/client/test/index.spec.ts b/packages/client/test/index.spec.ts index 2b6d96f..59ed38e 100644 --- a/packages/client/test/index.spec.ts +++ b/packages/client/test/index.spec.ts @@ -1,21 +1,21 @@ /* eslint-env mocha */ import { generateKeyPair } from '@libp2p/crypto/keys' +import { start } 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 @@ -357,6 +357,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 = [{ From 6444de18e48e26620cae5bdf7f2910211925e63c Mon Sep 17 00:00:00 2001 From: achingbrain Date: Mon, 18 Nov 2024 16:04:43 +0000 Subject: [PATCH 2/5] chore: use default values --- packages/client/src/client.ts | 6 +++--- packages/client/src/constants.ts | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) delete mode 100644 packages/client/src/constants.ts diff --git a/packages/client/src/client.ts b/packages/client/src/client.ts index ba115d7..bc44672 100644 --- a/packages/client/src/client.ts +++ b/packages/client/src/client.ts @@ -9,7 +9,6 @@ import { ipnsValidator } from 'ipns/validator' import { parse as ndjson } from 'it-ndjson' import defer from 'p-defer' import PQueue from 'p-queue' -import { CACHE_NAME } from './constants.js' import { BadResponseError, InvalidRequestError } from './errors.js' import { DelegatedRoutingV1HttpApiClientContentRouting, DelegatedRoutingV1HttpApiClientPeerRouting } from './routings.js' import type { DelegatedRoutingV1HttpApiClient, DelegatedRoutingV1HttpApiClientInit, GetProvidersOptions, GetPeersOptions, GetIPNSOptions, PeerRecord } from './index.js' @@ -22,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 { @@ -57,7 +57,7 @@ export class DefaultDelegatedRoutingV1HttpApiClient implements DelegatedRoutingV this.contentRouting = new DelegatedRoutingV1HttpApiClientContentRouting(this) this.peerRouting = new DelegatedRoutingV1HttpApiClientPeerRouting(this) - this.cacheName = init.cacheName ?? CACHE_NAME + this.cacheName = init.cacheName ?? defaultValues.cacheName this.cacheTTL = init.cacheTTL ?? defaultValues.cacheTTL } diff --git a/packages/client/src/constants.ts b/packages/client/src/constants.ts deleted file mode 100644 index e92e118..0000000 --- a/packages/client/src/constants.ts +++ /dev/null @@ -1 +0,0 @@ -export const CACHE_NAME = 'delegated-routing-v1-cache' From 00fac66b1c2080d663b2bf3441d641df82694906 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Mon, 18 Nov 2024 16:21:45 +0000 Subject: [PATCH 3/5] chore: add start method to interface --- packages/client/src/index.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 41bc501..5659afb 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -191,11 +191,17 @@ export interface DelegatedRoutingV1HttpApiClient { */ putIPNS(libp2pKey: CID, record: IPNSRecord, options?: AbortOptions): Promise + /** + * Create the request/response cache used to ensure duplicate requests aren't + * made for the same data + */ + start(): Promise + /** * Shut down any currently running HTTP requests and clear up any resources * that are in use */ - stop(): void + stop(): Promise } /** From b8eeac6b68dd49d850276baa690c7fcd6f6a319b Mon Sep 17 00:00:00 2001 From: achingbrain Date: Mon, 18 Nov 2024 16:32:28 +0000 Subject: [PATCH 4/5] chore: fix linting --- packages/client/test/index.spec.ts | 11 +++++------ packages/client/test/routings.spec.ts | 16 +++++++--------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/client/test/index.spec.ts b/packages/client/test/index.spec.ts index 59ed38e..1ffff14 100644 --- a/packages/client/test/index.spec.ts +++ b/packages/client/test/index.spec.ts @@ -1,7 +1,7 @@ /* eslint-env mocha */ import { generateKeyPair } from '@libp2p/crypto/keys' -import { start } from '@libp2p/interface' +import { start, stop } from '@libp2p/interface' import { peerIdFromPrivateKey, peerIdFromString } from '@libp2p/peer-id' import { multiaddr } from '@multiformats/multiaddr' import { expect } from 'aegir/chai' @@ -20,14 +20,13 @@ const serverUrl = process.env.ECHO_SERVER 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 () => { @@ -396,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) }) }) diff --git a/packages/client/test/routings.spec.ts b/packages/client/test/routings.spec.ts index fb7cb62..9dac3b6 100644 --- a/packages/client/test/routings.spec.ts +++ b/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', () => { From a446b52210227e1db18fee5de782877f7f44666a Mon Sep 17 00:00:00 2001 From: achingbrain Date: Mon, 18 Nov 2024 16:42:21 +0000 Subject: [PATCH 5/5] chore: more linting --- packages/interop/test/index.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/interop/test/index.spec.ts b/packages/interop/test/index.spec.ts index 62a09c9..7f2a2ec 100644 --- a/packages/interop/test/index.spec.ts +++ b/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()