Skip to content

Commit

Permalink
Fix customer issue: ReadOnlyError in liveQuery.
Browse files Browse the repository at this point in the history
Problem was that schedulePhysicalTick() in promise.js did derive the async context into arbritary promise continuations.
  • Loading branch information
dfahlander committed Jul 6, 2023
1 parent 26c4840 commit 988d73f
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 39 deletions.
42 changes: 11 additions & 31 deletions src/helpers/promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,26 +70,9 @@ var stack_being_generated = false;
db.ready().then() for every operation to make sure the indexedDB event is started in an
indexedDB-compatible emulated micro task loop.
*/
var schedulePhysicalTick = resolvedGlobalPromise ?
() => {resolvedGlobalPromise.then(physicalTick);}
:
_global.setImmediate ?
// setImmediate supported. Those modern platforms also supports Function.bind().
setImmediate.bind(null, physicalTick) :
_global.MutationObserver ?
// MutationObserver supported
() => {
var hiddenDiv = document.createElement("div");
(new MutationObserver(() => {
physicalTick();
hiddenDiv = null;
})).observe(hiddenDiv, { attributes: true });
hiddenDiv.setAttribute('i', '1');
} :
// No support for setImmediate or MutationObserver. No worry, setTimeout is only called
// once time. Every tick that follows will be our emulated micro tick.
// Could have uses setTimeout.bind(null, 0, physicalTick) if it wasnt for that FF13 and below has a bug
()=>{setTimeout(physicalTick,0);};
function schedulePhysicalTick() {
queueMicrotask(physicalTick);
}

// Configurable through Promise.scheduler.
// Don't export because it would be unsafe to let unknown
Expand Down Expand Up @@ -543,11 +526,15 @@ function linkToPreviousPromise(promise, prev) {
}
}

/* The callback to schedule with setImmediate() or setTimeout().
/* The callback to schedule with queueMicrotask().
It runs a virtual microtick and executes any callback registered in microtickQueue.
*/
function physicalTick() {
beginMicroTickScope() && endMicroTickScope();
usePSD(globalPSD, ()=>{
// Make sure to reset the async context to globalPSD before
// executing any of the microtick subscribers.
beginMicroTickScope() && endMicroTickScope();
});
}

export function beginMicroTickScope() {
Expand Down Expand Up @@ -758,7 +745,7 @@ function switchToZone (targetZone, bEnteringZone) {
if (bEnteringZone ? task.echoes && (!zoneEchoes++ || targetZone !== PSD) : zoneEchoes && (!--zoneEchoes || targetZone !== PSD)) {
// Enter or leave zone asynchronically as well, so that tasks initiated during current tick
// will be surrounded by the zone when they are invoked.
enqueueNativeMicroTask(bEnteringZone ? zoneEnterEcho.bind(null, targetZone) : zoneLeaveEcho);
queueMicrotask(bEnteringZone ? zoneEnterEcho.bind(null, targetZone) : zoneLeaveEcho);
}
if (targetZone === PSD) return;

Expand Down Expand Up @@ -822,13 +809,6 @@ export function usePSD (psd, fn, a1, a2, a3) {
}
}

function enqueueNativeMicroTask (job) {
//
// Precondition: nativePromiseThen !== undefined
//
nativePromiseThen.call(resolvedNativePromise, job);
}

function nativeAwaitCompatibleWrap(fn, zone, possibleAwait, cleanup) {
return typeof fn !== 'function' ? fn : function () {
var outerZone = PSD;
Expand All @@ -838,7 +818,7 @@ function nativeAwaitCompatibleWrap(fn, zone, possibleAwait, cleanup) {
return fn.apply(this, arguments);
} finally {
switchToZone(outerZone, false);
if (cleanup) enqueueNativeMicroTask(decrementExpectedAwaits);
if (cleanup) queueMicrotask(decrementExpectedAwaits);
}
};
}
Expand Down
21 changes: 14 additions & 7 deletions src/live-query/live-query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import {
DEXIE_STORAGE_MUTATED_EVENT_NAME,
} from '../globals/global-events';
import {
beginMicroTickScope,
decrementExpectedAwaits,
endMicroTickScope,
incrementExpectedAwaits,
newScope,
PSD,
Expand Down Expand Up @@ -36,14 +38,19 @@ export function liveQuery<T>(querier: () => T | Promise<T>): IObservable<T> {
const observable = new Observable<T>((observer) => {
const scopeFuncIsAsync = isAsyncFunction(querier);
function execute(ctx: LiveQueryContext) {
if (scopeFuncIsAsync) {
incrementExpectedAwaits();
}
const rv = newScope(querier, ctx);
if (scopeFuncIsAsync) {
(rv as Promise<any>).finally(decrementExpectedAwaits);
const wasRootExec = beginMicroTickScope(); // Performance: Avoid starting a new microtick scope within the async context.
try {
if (scopeFuncIsAsync) {
incrementExpectedAwaits();
}
const rv = newScope(querier, ctx);
if (scopeFuncIsAsync) {
(rv as Promise<any>).finally(decrementExpectedAwaits);
}
return rv;
} finally {
wasRootExec && endMicroTickScope(); // Given that we created the microtick scope, we must also end it.
}
return rv;
}

let closed = false;
Expand Down
56 changes: 55 additions & 1 deletion test/tests-misc.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import Dexie from 'dexie';
import Dexie, { liveQuery } from 'dexie';
import {module, stop, start, asyncTest, equal, deepEqual, ok} from 'QUnit';
import {resetDatabase, spawnedTest, promisedTest, supports, isIE, isEdge} from './dexie-unittest-utils';

Expand Down Expand Up @@ -421,3 +421,57 @@ promisedTest("Issue #1333 - uniqueKeys on virtual index should produce unique re
const result = await db.metrics.orderBy("name").uniqueKeys();
ok(result.length === 2, `Unexpected array length ${result.length} from uniqueKeys on virtual index, expected 2. Got ${result.join(',')}`);
});

/** Try to reproduce customer issue where ReadonlyError was thrown when using liveQuery.
* This repro is not good enough though as it doesn't fail in dexie@4.0.1-alpha.23.
* Probably need to reproduce it in a more complex scenario with
* multiple liveQueries and transactions running in parallel.
* However, the issue is fixed with this same commit which will be
* dexie@4.0.1-alpha.24. It is verified with customer application.
*
* Keeping the test in case there will be time to try to improve it later.
*/
promisedTest("Issue - ReadonlyError thrown in liveQuery despite user did not do write transactions", async () => {
// Encapsulating the code in a string to avoid transpilation. We need native await here to trigger bug.
ok(!Promise.PSD, "Must not be within async context when starting");
ok(db.isOpen(), "DB must be open when starting");
await new Promise(resolve => setTimeout(resolve, 10));
const F = new Function('ok', 'equal', 'Dexie', 'db', 'liveQuery', `
ok(true, "Got here");
return (async ()=>{
let physicalTickScheduled = false;
const observable = liveQuery(async () => {
console.debug("liveQuery executing");
//debugger;
const result = db.transaction('r', 'metrics', async () => db.metrics.toArray());
physicalTickScheduled = true;
console.debug("physicalTick executed by toArray");
return await result;
});
const subscription = observable.subscribe({
next: function (result) {
ok(true, "Got next result from observable");
}
});
ok(!!physicalTickScheduled, "physicalTick is scheduled at this point");
console.debug("liveQuery subscribed. Now doing transaction immediately - it will push to microtickQueue");
try {
//debugger;
//db.transaction('rw', db.metrics, () => {
// const x = Promise.PSD;
await db.metrics.add({ id: "id1", name: "a", time: 1 }).then(() => {
//debugger;
return db.metrics.update("id1", {name: "b"});
});
//});
console.debug("Transaction succeeded");
ok(true, "Successfully executed transaction");
} catch (error) {
console.debug("Transaction failed");
ok(false, 'Failed to execute transaction due to ' + error);
}
subscription.unsubscribe();
})();
`);
return F(ok, equal, Dexie, db, liveQuery).catch(err => ok(false, 'final catch: '+err));
});

0 comments on commit 988d73f

Please sign in to comment.