From c2fdb4b28eaba8537de8dc379361d4b8024cbad3 Mon Sep 17 00:00:00 2001 From: Roma Sosnovsky Date: Tue, 22 Sep 2026 13:34:05 +0300 Subject: [PATCH] #6297 Improve promises handling in ExpirationCache --- extension/js/common/core/expiration-cache.ts | 39 ++++--- extension/js/common/downloader.ts | 85 +++------------ extension/js/common/message-renderer.ts | 56 ++++------ .../unit-ExpirationCache.js | 100 ++++++++++++++---- 4 files changed, 142 insertions(+), 138 deletions(-) diff --git a/extension/js/common/core/expiration-cache.ts b/extension/js/common/core/expiration-cache.ts index 17cfc61c120..9c5a42a3dce 100644 --- a/extension/js/common/core/expiration-cache.ts +++ b/extension/js/common/core/expiration-cache.ts @@ -9,12 +9,27 @@ import { Env } from '../browser/env.js'; */ type ExpirationCacheType = { value: V; expiration: number }; export class ExpirationCache { + private readonly pendingRequests = new Map>(); + public constructor( public prefix: string, public expirationTicks: number ) {} - public set = async (key: string, value?: V, expiration?: number) => { + public getOrCreate = (key: string, create: () => Promise): Promise => { + const existing = this.pendingRequests.get(key); + if (existing) return existing; + const pending = (async () => { + const cached = await this.get(key); + if (cached !== undefined) return cached; + const value = await create(); + await this.set(key, value); + return value; + })(); + return this.track(key, pending); + }; + + public set = async (key: string, value?: V, expiration?: number): Promise => { if (Env.isContentScript()) { // Get chrome storage data from content script not allowed // Need to get data from service worker @@ -29,8 +44,8 @@ export class ExpirationCache { ); return; } - if (value) { - const expirationVal = { value, expiration: expiration || Date.now() + this.expirationTicks }; + if (value !== undefined) { + const expirationVal = { value, expiration: expiration ?? Date.now() + this.expirationTicks }; await storageSet('session', { [this.getPrefixedKey(key)]: expirationVal }); } else { await storageRemove('session', [this.getPrefixedKey(key)]); @@ -38,6 +53,8 @@ export class ExpirationCache { }; public get = async (key: string): Promise => { + const pending = this.pendingRequests.get(key); + if (pending) return await pending; if (Env.isContentScript()) { // Get chrome storage data from content script not allowed // Need to get data from service worker @@ -79,6 +96,7 @@ export class ExpirationCache { const keysToDelete: string[] = []; const entries = (await storageGetAll('session')) as Record>; for (const key of Object.keys(entries)) { + if (!key.startsWith(`${this.prefix}_`)) continue; const value = entries[key]; if (value.expiration <= Date.now() || additionalPredicate(key, value.value)) { keysToDelete.push(key); @@ -89,15 +107,12 @@ export class ExpirationCache { } }; - // await the value if it's a promise and remove from cache in case of exception - // the value is provided along with the key as parameter to eliminate possibility of a missing (expired) record - public await = async (key: string, value: V): Promise => { - try { - return value; - } catch (e) { - if ((await this.get(key)) === value) await this.set(key); // remove faulty record - return Promise.reject(e as Error); - } + private track = (key: string, pending: Promise): Promise => { + const tracked = pending.finally(() => { + if (this.pendingRequests.get(key) === tracked) this.pendingRequests.delete(key); + }); + this.pendingRequests.set(key, tracked); + return tracked; }; private getPrefixedKey = (key: string) => { diff --git a/extension/js/common/downloader.ts b/extension/js/common/downloader.ts index 4aa3d988079..f93df40bc13 100644 --- a/extension/js/common/downloader.ts +++ b/extension/js/common/downloader.ts @@ -7,12 +7,11 @@ import { Gmail } from './api/email-provider/gmail/gmail.js'; import { Attachment, Attachment$treatAs } from './core/attachment.js'; import { Buf } from './core/buf.js'; import { ExpirationCache } from './core/expiration-cache.js'; -import { Catch } from './platform/catch.js'; export class Downloader { - private readonly chunkDownloads = new ExpirationCache>('chunk', 2 * 60 * 60 * 1000); // 2 hours - private readonly fullMessages = new ExpirationCache>('full_message', 24 * 60 * 60 * 1000); // 24 hours - private readonly rawMessages = new ExpirationCache>('raw_message', 24 * 60 * 60 * 1000); // 24 hours + private readonly chunkDownloads = new ExpirationCache('chunk', 2 * 60 * 60 * 1000); // 2 hours + private readonly fullMessages = new ExpirationCache('full_message', 24 * 60 * 60 * 1000); // 24 hours + private readonly rawMessages = new ExpirationCache('raw_message', 24 * 60 * 60 * 1000); // 24 hours public constructor(private readonly gmail: Gmail) {} @@ -32,84 +31,28 @@ export class Downloader { if (a.hasData()) { return { result: Promise.resolve(a.getData()) }; } - // Couldn't use caching mechanism in firefox because in firefox it throws below error - // because we tried to send promise with chrome.runtime and firefox doesn't support it - // https://github.com/FlowCrypt/flowcrypt-browser/pull/5651#issuecomment-2054128442 - // Thrown[object]Error: Permission denied to access property "constructor" error - // Need to remove below code once firefox supports it - if (Catch.isFirefox()) { + const result = this.chunkDownloads.getOrCreate(a.id ?? '', async () => { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return { result: this.gmail.attachmentGetChunk(a.msgId!, a.id!, treatAs) }; - } - // Couldn't use async await for chunkDownloads.get - // because if we call `await chunkDownloads.get` - // then return type becomes Buf|undfined instead of Promise|undfined - return new Promise((resolve, reject) => { - this.chunkDownloads - .get(a.id ?? '') - .then(async download => { - if (!download || Object.keys(download).length < 1) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - download = this.gmail.attachmentGetChunk(a.msgId!, a.id!, treatAs); - await this.chunkDownloads.set(a.id ?? '', download); - } - resolve({ result: download }); - }) - .catch((e: unknown) => { - reject(e instanceof Error ? e : new Error(Catch.stringify(e))); - }); + const chunk = await this.gmail.attachmentGetChunk(a.msgId!, a.id!, treatAs); + // Background messaging can return an object of byte values without the Buf prototype. + const data = chunk instanceof Buf ? chunk : new Buf(Object.values(chunk)); + return data.toBase64Str(); }); + return { result: result.then(encoded => Buf.fromBase64Str(encoded)) }; }; public waitForAttachmentChunkDownload = async (a: Attachment, treatAs: Attachment$treatAs) => { if (a.hasData()) return a.getData(); - if (Catch.isFirefox()) { - const res = await this.queueAttachmentChunkDownload(a, treatAs); - return await res.result; - } - return this.chunkDownloads.await(a.id ?? '', (await this.queueAttachmentChunkDownload(a, treatAs)).result); + const { result } = await this.queueAttachmentChunkDownload(a, treatAs); + return await result; }; public msgGetRaw = async (msgId: string): Promise => { - if (Catch.isFirefox()) { - const msgDownload = await this.gmail.msgGet(msgId, 'raw'); - return msgDownload.raw || ''; - } - return new Promise((resolve, reject) => { - this.rawMessages - .get(msgId) - .then(async msgDownload => { - if (!msgDownload || Object.keys(msgDownload).length < 1) { - msgDownload = this.gmail.msgGet(msgId, 'raw'); - await this.rawMessages.set(msgId, msgDownload); - } - const msg = await this.rawMessages.await(msgId, msgDownload); - resolve(msg.raw || ''); - }) - .catch((e: unknown) => { - reject(e instanceof Error ? e : new Error(Catch.stringify(e))); - }); - }); + const msg = await this.rawMessages.getOrCreate(msgId, () => this.gmail.msgGet(msgId, 'raw')); + return msg.raw || ''; }; public msgGetFull = async (msgId: string): Promise => { - if (Catch.isFirefox()) { - return await this.gmail.msgGet(msgId, 'full'); - } - return new Promise((resolve, reject) => { - this.fullMessages - .get(msgId) - .then(async msgDownload => { - if (!msgDownload || Object.keys(msgDownload).length < 1) { - msgDownload = this.gmail.msgGet(msgId, 'full'); - await this.fullMessages.set(msgId, msgDownload); - } - const msg = await this.rawMessages.await(msgId, msgDownload); - resolve(msg); - }) - .catch((e: unknown) => { - reject(e instanceof Error ? e : new Error(Catch.stringify(e))); - }); - }); + return await this.fullMessages.getOrCreate(msgId, () => this.gmail.msgGet(msgId, 'full')); }; } diff --git a/extension/js/common/message-renderer.ts b/extension/js/common/message-renderer.ts index 195e391d690..1add16e3d23 100644 --- a/extension/js/common/message-renderer.ts +++ b/extension/js/common/message-renderer.ts @@ -32,7 +32,6 @@ import { LoaderContextInterface } from './loader-context-interface.js'; import { Gmail } from './api/email-provider/gmail/gmail.js'; import { ApiErr } from './api/shared/api-error.js'; import { isCustomerUrlFesUsed } from './helpers.js'; -import { ExpirationCache } from './core/expiration-cache.js'; type ProcessedMessage = { body: MessageBody; @@ -43,7 +42,8 @@ type ProcessedMessage = { export class MessageRenderer { public readonly downloader: Downloader; - private readonly processedMessages = new ExpirationCache>('processed_message', 24 * 60 * 60 * 1000); // 24 hours + private readonly processedMessages = new Map>(); + private readonly resolvedMessages = new Map(); private constructor( private readonly acctEmail: string, @@ -310,17 +310,7 @@ export class MessageRenderer { if (this.debug) { console.debug('processAttachment() try -> awaiting chunk + awaiting type'); } - let data = await this.downloader.waitForAttachmentChunkDownload(a, treatAs); - // For some reason, it sometimes doesn't return Buf and instead returns object - // Need to convert it to Buf - // todo: this is a temporary fix, should be removed - // https://github.com/FlowCrypt/flowcrypt-browser/pull/5607#discussion_r1540198173 - if (!(data instanceof Buf)) { - const att = Object.entries(data).map(entry => { - return entry[1] as number; - }); - data = new Buf(att); - } + const data = await this.downloader.waitForAttachmentChunkDownload(a, treatAs); const openpgpType = MsgUtil.type({ data }); if (openpgpType?.type === 'publicKey' && openpgpType.armored) { // todo: publicKey attachment can't be too big, so we could do preparePubkey() call (checking file length) right here @@ -420,35 +410,27 @@ export class MessageRenderer { }; public deleteExpired = (): void => { - void this.processedMessages.deleteExpired(); + for (const [key, entry] of this.resolvedMessages) { + if (entry.expiration <= Date.now()) this.resolvedMessages.delete(key); + } this.downloader.deleteExpired(); }; public msgGetProcessed = async (msgId: string): Promise => { - // Couldn't use caching mechanism in firefox - // https://github.com/FlowCrypt/flowcrypt-browser/pull/5651#issuecomment-2054128442 - if (Catch.isFirefox()) { - return await this.processFull(await this.downloader.msgGetFull(msgId)); - } - // Couldn't use async await for chunkDownloads.get - // because if we call `await chunkDownloads.get` - // then return type becomes Buf|undfined instead of Promise|undfined - return new Promise((resolve, reject) => { - this.processedMessages - .get(msgId) - .then(async processed => { - if (!processed || Object.keys(processed).length < 1) { - processed = (async () => { - return this.processFull(await this.downloader.msgGetFull(msgId)); - })(); - } - await this.processedMessages.set(msgId, processed); - resolve(await this.processedMessages.await(msgId, processed)); - }) - .catch((e: unknown) => { - reject(e instanceof Error ? e : new Error(Catch.stringify(e))); - }); + const cached = this.resolvedMessages.get(msgId); + if (cached && cached.expiration > Date.now()) return cached.value; + const existing = this.processedMessages.get(msgId); + if (existing) return await existing; + // Processed messages contain class instances and must not cross extension storage. + const pending = (async () => { + const value = await this.processFull(await this.downloader.msgGetFull(msgId)); + this.resolvedMessages.set(msgId, { value, expiration: Date.now() + 24 * 60 * 60 * 1000 }); + return value; + })().finally(() => { + this.processedMessages.delete(msgId); }); + this.processedMessages.set(msgId, pending); + return await pending; }; private processFull = async (fullMsg: GmailRes.GmailMsg): Promise => { diff --git a/test/source/tests/browser-unit-tests/unit-ExpirationCache.js b/test/source/tests/browser-unit-tests/unit-ExpirationCache.js index 8c35ae13be0..bfa4918a427 100644 --- a/test/source/tests/browser-unit-tests/unit-ExpirationCache.js +++ b/test/source/tests/browser-unit-tests/unit-ExpirationCache.js @@ -20,26 +20,90 @@ BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache] entry expires after configured i return 'pass'; })(); -BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache.await] removes rejected promises from cache`); +BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache] awaits rejected promises and allows retry`); (async () => { - const cache = new ExpirationCache('test-cache-promise', 24 * 60 * 60 * 1000); // 24 hours - const rejectionPromise = Promise.reject(Error('test-error')); - cache.set('test-key', rejectionPromise); - let cacheValue = cache.get('test-key'); - if (cacheValue?.length) { - throw Error(`Expected cache value to be undefined but got ${JSON.stringify(cacheValue)}`); - } // next call simply returns undefined - const fulfilledPromise = Promise.resolve('new-test-value'); - cache.set('test-key', fulfilledPromise); - cacheValue = await cache.await('test-key', fulfilledPromise); - // good value is returned indefinitely - if (cacheValue !== 'new-test-value') { - throw Error(`Expected cache value to be "new-test-value" but got ${cacheValue}`); + const cache = new ExpirationCache('test-cache-promise', 60000); + const error = Error('test-error'); + const creating = cache.getOrCreate('test-key', async () => { + throw error; + }); + const getting = cache.get('test-key'); + const results = await Promise.allSettled([creating, getting]); + if (results.some(result => result.status !== 'rejected' || result.reason !== error)) { + throw Error('Both getOrCreate and get must reject with the original error'); } - cacheValue = await cache.await('test-key', fulfilledPromise); - // good value is returned indefinitely - if (cacheValue !== 'new-test-value') { - throw Error(`Expected cache value to be "new-test-value" but got ${cacheValue}`); + if ((await cache.get('test-key')) !== undefined) throw Error('Rejected promise was retained'); + await cache.set('test-key', 'new-test-value'); + if ((await cache.get('test-key')) !== 'new-test-value') throw Error('Retry did not persist resolved value'); + const reloaded = new ExpirationCache('test-cache-promise', 60000); + if ((await reloaded.get('test-key')) !== 'new-test-value') throw Error('Stored value was not serializable'); + return 'pass'; +})(); + +BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache] deduplicates concurrent misses and retries failures`); +(async () => { + const cache = new ExpirationCache('test-cache-dedup', 60000); + let calls = 0; + const create = async () => { + calls++; + throw Error('test-error'); + }; + const first = cache.getOrCreate('key', create); + const second = cache.getOrCreate('key', create); + if (first !== second) throw Error('Concurrent requests did not share a promise'); + const results = await Promise.allSettled([first, second]); + if (calls !== 1 || results.some(result => result.status !== 'rejected')) throw Error('Failure was not deduplicated'); + const value = await cache.getOrCreate('key', async () => { + calls++; + return 'success'; + }); + if (value !== 'success' || calls !== 2) throw Error('Failure prevented retry'); + await cache.getOrCreate('key', async () => { + throw Error('Should read session storage'); + }); + return 'pass'; +})(); + +BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache] downloader retries full messages without evicting raw messages`); +(async () => { + const { Downloader } = await import('/js/common/downloader.js'); + let fullCalls = 0; + let rawCalls = 0; + const downloader = new Downloader({ + msgGet: async (id, format) => { + if (format === 'raw') { + rawCalls++; + return { id, raw: 'raw-value' }; + } + fullCalls++; + if (fullCalls === 1) throw Error('test-error'); + return { id, payload: { mimeType: 'text/plain' } }; + }, + }); + const id = 'cache-rejection-test'; + await downloader.msgGetRaw(id); + const results = await Promise.allSettled([downloader.msgGetFull(id), downloader.msgGetFull(id)]); + if (fullCalls !== 1 || results.some(result => result.status !== 'rejected')) throw Error('Full requests were not deduplicated'); + if ((await downloader.msgGetFull(id)).id !== id || fullCalls !== 2) throw Error('Full request did not retry'); + if ((await downloader.msgGetRaw(id)) !== 'raw-value' || rawCalls !== 1) throw Error('Raw cache was evicted'); + return 'pass'; +})(); + +BROWSER_UNIT_TEST_NAME(`[unit][ExpirationCache] downloader restores serialized and cached attachment buffers`); +(async () => { + const { Downloader } = await import('/js/common/downloader.js'); + let calls = 0; + const downloader = new Downloader({ + attachmentGetChunk: async () => { + calls++; + return JSON.parse(JSON.stringify(Buf.fromUtfStr('attachment data'))); + }, + }); + const attachment = { id: 'cache-buffer-test', msgId: 'message', hasData: () => false }; + const first = await downloader.waitForAttachmentChunkDownload(attachment, 'plainFile'); + const second = await downloader.waitForAttachmentChunkDownload(attachment, 'plainFile'); + if (calls !== 1 || first.toUtfStr() !== 'attachment data' || second.toUtfStr() !== 'attachment data') { + throw Error('Cached buffer did not retain its data and methods'); } return 'pass'; })();