Skip to content

Make target.loadedFromSource the sole cache-disposition signal; drop the Context mirror (#1576) #1590

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

Closed
kriszyp wants to merge 1 commit into from
Closed

Make target.loadedFromSource the sole cache-disposition signal; drop the Context mirror (#1576) #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion resources/RequestTarget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ export class RequestTarget extends URLSearchParams {
declare previousResidency?: string[];

// Action tracking
/** Cache disposition of a get on a caching table; also mirrored onto the request Context. */
/** Cache disposition of this get on a caching table: true if loaded from the source, false if
* served from cache. Set per-get; read it on the RequestTarget you passed to the get. */
declare loadedFromSource?: boolean;
declare createdNewId?: string;

Expand Down
6 changes: 0 additions & 6 deletions resources/ResourceInterface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,6 @@ export interface Context {
sourceApply?: boolean;
originatingOperation?: OperationFunctionName;
previousResidency?: string[];
/** Cache disposition of the most recent get on a caching table in this context: true if the get
* fetched from source — including when a source error fell back to a stale record (staleIfError);
* false if served from cache — including stale-while-revalidate responses (the source fetch
* continues in the background) and waits on another request's in-flight source fetch.
* Subsequent gets in the same context overwrite it. */
loadedFromSource?: boolean;
nodeName?: string;
resourceCache?: Map<Id, any>;
_freezeRecords?: boolean; // until v5, we conditionally freeze records for back-compat
Expand Down
24 changes: 9 additions & 15 deletions resources/Table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ export function makeTable(options) {
// return 504 (rather than 404) if there is no content and the cache-control header
// dictates not to go to source
if (!this.doesExist()) throw new ServerError('Entry is not cached', 504);
if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached
if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached
} else if (resourceOptions?.ensureLoaded) {
const loadingFromSource = ensureLoadedFromSource(
(this.constructor as any).source,
Expand All @@ -743,7 +743,7 @@ export function makeTable(options) {
TableResource._updateResource(this, entry);
return this;
});
} else if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached
} else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached
}
return this;
}
Expand Down Expand Up @@ -776,7 +776,7 @@ export function makeTable(options) {
this.#record = entry.value;
this.#version = entry.version;
});
} else if (hasSourceGet) setLoadedFromSource(undefined, this.getContext(), false); // mark it as cached
}
}
// #section: lifecycle-admin
static getNewId(): any {
Expand Down Expand Up @@ -1224,7 +1224,7 @@ export function makeTable(options) {
// return 504 (rather than 404) if there is no content and the cache-control header
// dictates not to go to source
if (!entry?.value) throw new ServerError('Entry is not cached', 504);
if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached
if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached
} else if (ensureLoaded) {
const loadingFromSource = ensureLoadedFromSource(
constructor.source,
Expand All @@ -1237,7 +1237,7 @@ export function makeTable(options) {
if (loadingFromSource) {
txn?.disregardReadTxn(); // this could take some time, so don't keep the transaction open if possible
return loadingFromSource.then((entry) => entry?.value);
} else if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached
} else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached
}
return entry?.value;
});
Expand Down Expand Up @@ -4555,16 +4555,10 @@ export function makeTable(options) {
}
}

function setLoadedFromSource(
target: RequestTarget | undefined,
context: Context | undefined,
loadedFromSource: boolean
) {
// mirror the flag onto the context: callers that pass a plain id get an internal
// RequestTarget they never see, so the context is their only way to observe cache disposition (#1571)
function setLoadedFromSource(target: RequestTarget | undefined, loadedFromSource: boolean) {
// cache disposition is a per-get result, recorded on the RequestTarget of the get (#1576)
// target may be a primitive id on instance-API calls, which can't hold the flag
if (target && typeof target === 'object') target.loadedFromSource = loadedFromSource;
if (context) context.loadedFromSource = loadedFromSource;
}
function ensureLoadedFromSource(source: typeof TableResource, id, entry, context, resource?, target?) {
if (context?.onlyIfCached) {
Expand Down Expand Up @@ -4798,7 +4792,7 @@ export function makeTable(options) {
whenResolved(getFromSource(source, id, primaryStore.getEntry(id), context, target));
else {
// served from cache after waiting for another request to resolve
setLoadedFromSource(target, context, false);
setLoadedFromSource(target, false);
whenResolved(entry);
}
};
Expand All @@ -4813,7 +4807,7 @@ export function makeTable(options) {
});
}
// lock acquired — this request will actually load from source
setLoadedFromSource(target, context, true);
setLoadedFromSource(target, true);

const existingRecord = existingEntry?.value;
// it is important to remember that this is _NOT_ part of the current transaction; nothing is changing
Expand Down
44 changes: 15 additions & 29 deletions unitTests/resources/caching.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,39 +133,24 @@ describe('Caching', () => {
assert.equal(target23.loadedFromSource, true);
});

it('loadedFromSource is observable on the context with a plain id', async function () {
// with a plain id, the static get dispatch mints an internal RequestTarget the caller
// never sees, so the flag must be mirrored onto the context (#1571)
CachingTable.setTTLExpiration(30);
await CachingTable.invalidate(31);
let context = {};
let result = await CachingTable.get(31, context);
assert.equal(result.id, 31);
assert.equal(context.loadedFromSource, true);
context = {};
result = await CachingTable.get(31, context);
assert.equal(result.id, 31);
assert.equal(context.loadedFromSource, false);
context = { onlyIfCached: true };
result = await CachingTable.get(31, context);
assert.equal(result.id, 31);
assert.equal(context.loadedFromSource, false);
});

it('loadedFromSource is observable on the context with loadAsInstance = false', async function () {
it('loadedFromSource is observable on the target with loadAsInstance = false (#1576)', async function () {
// disposition is recorded on the RequestTarget of the get; verify the loadAsInstance=false
// value path marks it on both cache miss (true) and cache hit (false)
const previousLoadAsInstance = CachingTable.loadAsInstance;
try {
CachingTable.loadAsInstance = false;
CachingTable.setTTLExpiration(30);
await CachingTable.invalidate(32);
let context = {};
let result = await CachingTable.get(32, context);
let target = new RequestTarget();
target.id = 32;
let result = await CachingTable.get(target);
assert.equal(result.id, 32);
assert.equal(context.loadedFromSource, true);
context = {};
result = await CachingTable.get(32, context);
assert.equal(target.loadedFromSource, true);
target = new RequestTarget();
target.id = 32;
result = await CachingTable.get(target);
assert.equal(result.id, 32);
assert.equal(context.loadedFromSource, false);
assert.equal(target.loadedFromSource, false);
} finally {
CachingTable.loadAsInstance = previousLoadAsInstance;
}
Expand Down Expand Up @@ -266,10 +251,11 @@ describe('Caching', () => {
events = [];
await new Promise((resolve) => setTimeout(resolve, 10));
// should be stale but not evicted
const swrContext = {};
let result = await CachingTableStaleWhileRevalidate.get(23, swrContext);
const swrTarget = new RequestTarget();
swrTarget.id = 23;
let result = await CachingTableStaleWhileRevalidate.get(swrTarget);
assert(result); // should exist in database even though it is stale
assert.equal(swrContext.loadedFromSource, false); // stale value served from cache while revalidating
assert.equal(swrTarget.loadedFromSource, false); // stale value served from cache while revalidating
assert.equal(sourceRequests, 1); // the source request should be started
assert.equal(sourceResponses, 0); // the source request should not be completed yet
// the source request should be completed
Expand Down