Skip to content

#6297 Improve promises handling in ExpirationCache #6298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sosnovsky merged 2 commits into from
Sep 22, 2026
Merged

#6297 Improve promises handling in ExpirationCache #6298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2fdb4b
#6297 Improve promises handling in ExpirationCache
sosnovsky Sep 22, 2026
5ff54c2
Merge branch 'master' into 6297-expiration-cache-fix
martgil Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions extension/js/common/core/expiration-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,27 @@ import { Env } from '../browser/env.js';
*/
type ExpirationCacheType<V> = { value: V; expiration: number };
export class ExpirationCache<V> {
private readonly pendingRequests = new Map<string, Promise<V>>();

public constructor(
public prefix: string,
public expirationTicks: number
) {}

public set = async (key: string, value?: V, expiration?: number) => {
public getOrCreate = (key: string, create: () => Promise<V>): Promise<V> => {
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<void> => {
if (Env.isContentScript()) {
// Get chrome storage data from content script not allowed
// Need to get data from service worker
Expand All @@ -29,15 +44,17 @@ export class ExpirationCache<V> {
);
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)]);
}
};

public get = async (key: string): Promise<V | undefined> => {
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
Expand Down Expand Up @@ -79,6 +96,7 @@ export class ExpirationCache<V> {
const keysToDelete: string[] = [];
const entries = (await storageGetAll('session')) as Record<string, ExpirationCacheType<V>>;
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);
Expand All @@ -89,15 +107,12 @@ export class ExpirationCache<V> {
}
};

// 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<V> => {
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<V>): Promise<V> => {
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) => {
Expand Down
85 changes: 14 additions & 71 deletions extension/js/common/downloader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Promise<Buf>>('chunk', 2 * 60 * 60 * 1000); // 2 hours
private readonly fullMessages = new ExpirationCache<Promise<GmailRes.GmailMsg>>('full_message', 24 * 60 * 60 * 1000); // 24 hours
private readonly rawMessages = new ExpirationCache<Promise<GmailRes.GmailMsg>>('raw_message', 24 * 60 * 60 * 1000); // 24 hours
private readonly chunkDownloads = new ExpirationCache<string>('chunk', 2 * 60 * 60 * 1000); // 2 hours
private readonly fullMessages = new ExpirationCache<GmailRes.GmailMsg>('full_message', 24 * 60 * 60 * 1000); // 24 hours
private readonly rawMessages = new ExpirationCache<GmailRes.GmailMsg>('raw_message', 24 * 60 * 60 * 1000); // 24 hours

public constructor(private readonly gmail: Gmail) {}

Expand All @@ -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<Buf>|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<string> => {
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<GmailRes.GmailMsg> => {
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'));
};
}
56 changes: 19 additions & 37 deletions extension/js/common/message-renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,7 +42,8 @@ type ProcessedMessage = {

export class MessageRenderer {
public readonly downloader: Downloader;
private readonly processedMessages = new ExpirationCache<Promise<ProcessedMessage>>('processed_message', 24 * 60 * 60 * 1000); // 24 hours
private readonly processedMessages = new Map<string, Promise<ProcessedMessage>>();
private readonly resolvedMessages = new Map<string, { value: ProcessedMessage; expiration: number }>();

private constructor(
private readonly acctEmail: string,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<ProcessedMessage> => {
// 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<Buf>|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<ProcessedMessage> => {
Expand Down
100 changes: 82 additions & 18 deletions test/source/tests/browser-unit-tests/unit-ExpirationCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
})();
Loading