Skip to content
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.

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

Fix TransactionInactiveError on Safari #118

Merged
merged 29 commits into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
76381fb
open storage transactions synchronously
bwindels Sep 25, 2020
c68bafd
prototype using await to see if it makes any difference
bwindels Sep 25, 2020
ee4c132
add todo
bwindels Sep 26, 2020
b1f9cfd
cleanup storage errors a bit
bwindels Sep 29, 2020
4d23529
set promise polyfill before others
bwindels Sep 29, 2020
07fcf7e
also do this in try catch
bwindels Sep 29, 2020
c1ecaff
fix refactor typo
bwindels Sep 29, 2020
c529df1
also import this
bwindels Sep 29, 2020
919357b
more broken imports after refactor
bwindels Sep 29, 2020
163ca12
ignore abort error
bwindels Sep 29, 2020
7627a2b
add comment
bwindels Sep 29, 2020
e5b1cbb
prevent endless loop when restoring messages that were already sent
bwindels Sep 29, 2020
f993048
Merge branch 'master' into bwindels/idb-promises-txn
bwindels Sep 29, 2020
43d430f
remove unused storage modification functions
bwindels Sep 29, 2020
482b5f4
allow passing message to IDBRequestError
bwindels Sep 29, 2020
37690cf
track storage write requests internally, as we never await their promise
bwindels Sep 29, 2020
d5a52c3
these don't return a promise anymore
bwindels Sep 29, 2020
ddb14f4
we actually don't need to track write requests
bwindels Oct 1, 2020
93a7f99
Safari doesn't like the prepare txn still open when opening the sync txn
bwindels Oct 1, 2020
3c7125b
add (optional) logging for idb requests
bwindels Oct 1, 2020
d5a6a4d
todo comment
bwindels Oct 1, 2020
1117c77
note for future optimisation
bwindels Oct 1, 2020
300529b
write sync token first
bwindels Oct 1, 2020
f402e8c
typo/thinko in docs
bwindels Oct 1, 2020
b08b7e5
(failed) attempt at a repro case for the safari TransactionInactiveError
bwindels Oct 1, 2020
cb7da2b
dont need this anymore
bwindels Oct 1, 2020
9a4d478
change this back as well
bwindels Oct 1, 2020
c1df371
add some documentation for our idb investigations
bwindels Oct 1, 2020
bebdaad
log when we can't abort
bwindels Oct 1, 2020
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
73 changes: 73 additions & 0 deletions doc/INDEXEDDB.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
## Promises, async/await and indexedDB

Doesn't indexedDB close your transaction if you don't queue more requests from an idb event handler?
So wouldn't that mean that you can't use promises and async/await when using idb?

It used to be like this, and for IE11 on Win7 (not on Windows 10 strangely enough), it still is like this.
Here we manually flush the promise queue synchronously at the end of an idb event handler.

In modern browsers, indexedDB transactions should only be closed after flushing the microtask queue of the event loop,
which is where promises run.

Keep in mind that indexedDB events, just like any other DOM event, are fired as macro tasks.
Promises queue micro tasks, of which the queue is drained before proceeding to the next macro task.
This also means that if a transaction is completed, you will only receive the event once you are ready to process the next macro tasks.
That doesn't prevent any placed request from throwing TransactionInactiveError though.

## TransactionInactiveError in Safari

Safari doesn't fully follow the rules above, in that if you open a transaction,
you need to "use" (not sure if this means getting a store or actually placing a request) it straight away,
without waiting for any *micro*tasks. See comments about Safari at https://github.com/dfahlander/Dexie.js/issues/317#issue-178349994.

Another failure mode perceived in Hydrogen on Safari is that when the (readonly) prepareTxn in sync wasn't awaited to be completed before opening and using the syncTxn.
I haven't found any documentation online about this at all. Awaiting prepareTxn.complete() fixed the issue below. It's strange though the put does not fail.

What is happening below is:
- in the sync loop:
- we first open a readonly txn on inboundGroupSessions, which we don't use in the example below
- we then open a readwrite txn on session, ... (does not overlap with first txn)
- first the first incremental sync on a room (!YxKeAxtNcDZDrGgaMF:matrix.org) it seems to work well
- on a second incremental sync for that same room, the first get throws TransactionInactiveError for some reason.
- the put in the second incremental sync somehow did not throw.

So it looks like safari doesn't like (some) transactions still being active while a second one is being openened, even with non-overlapping stores.
For now I haven't awaited every read txn in the app, as this was the only place it fails, but if this pops up again in safari, we might have to do that.

Keep in mind that the `txn ... inactive` logs are only logged when the "complete" or "abort" events are processed,
which happens in a macro task, as opposed to all of our promises, which run in a micro task.
So the transaction is likely to have closed before it appears in the logs.

```
[Log] txn 4504181722375185 active on inboundGroupSessions
[Log] txn 861052256474256 active on session, roomSummary, roomState, roomMembers, timelineEvents, timelineFragments, pendingEvents, userIdentities, groupSessionDecryptions, deviceIdentities, outboundGroupSessions, operations, accountData
[Info] hydrogen_session_5286139994689036.session.put({"key":"sync","value":{"token":"s1572540047_757284957_7660701_602588550_435736037_1567300_101589125_347651623_132704","filterId":"2"}})
[Info] hydrogen_session_5286139994689036.userIdentities.get("@bwindels:matrix.org")
[Log] txn 4504181722375185 inactive
[Log] * applying sync response to room !YxKeAxtNcDZDrGgaMF:matrix.org ...
[Info] hydrogen_session_5286139994689036.roomMembers.put({"roomId":"!YxKeAxtNcDZDrGgaMF:matrix.org","userId":"@bwindels:matrix.org","membership":"join","avatarUrl":"mxc://matrix.org/aerWVfICBMcyFcEyREcivLuI","displayName":"Bruno","key":"!YxKeAxtNcDZDrGgaMF:matrix.org|@bwindels:matrix.org"})
[Info] hydrogen_session_5286139994689036.roomMembers.get("!YxKeAxtNcDZDrGgaMF:matrix.org|@bwindels:matrix.org")
[Info] hydrogen_session_5286139994689036.timelineEvents.add({"fragmentId":0,"eventIndex":2147483658,"roomId":"!YxKeAxtNcDZDrGgaMF:matrix.org","event":{"content":{"body":"haha","msgtype":"m.text"},"origin_server_ts":1601457573756,"sender":"@bwindels:matrix.org","type":"m.room.message","unsigned":{"age":8360},"event_id":"$eD9z73-lCpXBVby5_fKqzRZzMVHiPzKbE_RSZzqRKx0"},"displayName":"Bruno","avatarUrl":"mxc://matrix.org/aerWVfICBMcyFcEyREcivLuI","key":"!YxKeAxtNcDZDrGgaMF:matrix.org|00000000|8000000a","eventIdKey":"!YxKeAxtNcDZDrGgaMF:matrix.org|$eD9z73-lCpXBVby5_fKqzRZzMVHiPzKbE_RSZzqRKx0"})
[Info] hydrogen_session_5286139994689036.roomSummary.put({"roomId":"!YxKeAxtNcDZDrGgaMF:matrix.org","name":"!!!test8!!!!!!","lastMessageBody":"haha","lastMessageTimestamp":1601457573756,"isUnread":true,"encryption":null,"lastDecryptedEventKey":null,"isDirectMessage":false,"membership":"join","inviteCount":0,"joinCount":2,"heroes":null,"hasFetchedMembers":false,"isTrackingMembers":false,"avatarUrl":null,"notificationCount":5,"highlightCount":0,"tags":{"m.lowpriority":{}}})
[Log] txn 861052256474256 inactive
[Info] syncTxn committed!!

... two more unrelated sync responses ...

[Log] starting sync request with since s1572540191_757284957_7660742_602588567_435736063_1567300_101589126_347651632_132704 ...
[Log] txn 8104296957004707 active on inboundGroupSessions
[Log] txn 2233038992157489 active on session, roomSummary, roomState, roomMembers, timelineEvents, timelineFragments, pendingEvents, userIdentities, groupSessionDecryptions, deviceIdentities, outboundGroupSessions, operations, accountData
[Info] hydrogen_session_5286139994689036.session.put({"key":"sync","value":{"token":"s1572540223_757284957_7660782_602588579_435736078_1567300_101589130_347651633_132704","filterId":"2"}})
[Log] * applying sync response to room !YxKeAxtNcDZDrGgaMF:matrix.org ...
[Info] hydrogen_session_5286139994689036.roomMembers.get("!YxKeAxtNcDZDrGgaMF:matrix.org|@bwindels:matrix.org")
[Warning] stopping sync because of error
[Error] StorageError: get("!YxKeAxtNcDZDrGgaMF:matrix.org|@bwindels:matrix.org") failed on txn with stores accountData, deviceIdentities, groupSessionDecryptions, operations, outboundGroupSessions, pendingEvents, roomMembers, roomState, roomSummary, session, timelineEvents, timelineFragments, userIdentities on hydrogen_session_5286139994689036.roomMembers: (name: TransactionInactiveError) (code: 0) Failed to execute 'get' on 'IDBObjectStore': The transaction is inactive or finished.
(anonymous function)
asyncFunctionResume
(anonymous function)
promiseReactionJobWithoutPromise
promiseReactionJob
[Log] newStatus – "SyncError"
[Log] txn 8104296957004707 inactive
[Log] txn 2233038992157489 inactive
```
112 changes: 112 additions & 0 deletions prototypes/idb-promises-es6.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
</head>
<body>
<script type="text/javascript" src="promifill.js"></script>
<!-- <script src="https://cdn.jsdelivr.net/npm/promise-polyfill@8/dist/polyfill.min.js"></script> -->
<script type="text/javascript">
//window.Promise = Promifill;
function reqAsPromise(req) {
return new Promise(function (resolve, reject) {
req.onsuccess = function() {
resolve(req);
Promise.flushQueue && Promise.flushQueue();
};
req.onerror = function(e) {
reject(new Error("IDB request failed: " + e.target.error.message));
Promise.flushQueue && Promise.flushQueue();
};
});
}

function Storage(databaseName) {
this._databaseName = databaseName;
this._database = null;
}

Storage.prototype = {
open: function() {
const req = window.indexedDB.open(this._databaseName);
const self = this;
req.onupgradeneeded = function(ev) {
const db = ev.target.result;
const oldVersion = ev.oldVersion;
self._createStores(db, oldVersion);
};
return reqAsPromise(req).then(function() {
self._database = req.result;
});
},
openTxn: function(mode, storeName) {
const txn = this._database.transaction([storeName], mode);
const store = txn.objectStore(storeName);
return Promise.resolve(store);
},
_createStores: function(db) {
db.createObjectStore("foos", {keyPath: ["id"]});
}
};

function getAll(store) {
const request = store.openCursor();
const results = [];
return new Promise(function(resolve, reject) {
request.onsuccess = function(event) {
const cursor = event.target.result;
if(cursor) {
results.push(cursor.value);
cursor.continue();
} else {
resolve(results);
Promise.flushQueue && Promise.flushQueue();
}
};
request.onerror = function(e) {
reject(new Error("IDB request failed: " + e.target.error.message));
Promise.flushQueue && Promise.flushQueue();
};
});
}

async function main() {
try {
let storage = new Storage("idb-promises");
await storage.open();
const store = await storage.openTxn("readwrite", "foos");
store.clear();
store.add({id: 5, name: "foo"});
store.add({id: 6, name: "bar"});
console.log("all1", await getAll(store));
store.add({id: 7, name: "bazzz"});
console.log("all2", await getAll(store));
} catch(err) {
console.error(err.message + ": " + err.stack);
};
}
main();


/*

we basically want something like this for IE11/Win7:

return new Promise(function (resolve, reject) {
req.onsuccess = function() {
resolve(req);
Promise?.flushQueue();
};
req.onerror = function(e) {
reject(new Error("IDB request failed: " + e.target.error.message));
Promise?.flushQueue();
};
});

we don't have this problem on platforms with a native promise implementation, so we can just have our own (forked) promise polyfill?
*/
</script>
</body>
</html>

169 changes: 169 additions & 0 deletions prototypes/idb-promises-safari.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
</head>
<body>
<script type="text/javascript">
function reqAsPromise(req) {
return new Promise(function (resolve, reject) {
req.onsuccess = function() {
resolve(req.result);
};
req.onerror = function(e) {
reject(new Error("IDB request failed: " + req.error));
};
});
}
function txnAsPromise(txn) {
return new Promise(function (resolve, reject) {
txn.oncomplete = function() {
resolve(txn);
};
txn.onabort = function(e) {
reject(new Error("Transaction got aborted: " + txn.error));
};
});
}

const BrowserMutationObserver = window.MutationObserver || window.WebKitMutationObserver;

function useMutationObserver(flush) {
let iterations = 0;
const observer = new BrowserMutationObserver(flush);
const node = document.createTextNode('');
observer.observe(node, { characterData: true });

return () => {
node.data = (iterations = ++iterations % 2);
};
}

const wait = (function() {
let resolve = null;
const trigger = useMutationObserver(() => {
resolve();
});
return () => {
return new Promise(r => {
resolve = r;
trigger();
});
};
})();


var _resolve = Promise.resolve.bind(Promise);
var _then = Promise.prototype.then;

async function delay() {
return Promise.resolve();
// two consecutive macro tasks
//await new Promise(r => setImmediate(r));
// the next macro task will now be the complete event of the txn,
// so schedule another macro task to execute after that
//await new Promise(r => setImmediate(r));
//return;
// for (let i = 0; i < 1000; i+=1) {
// console.log("await...");
// await wait();
// }
let p = _resolve(0);
for (let i=0;i<10;++i) {
p = _then.call(p, x => x + 1);
}
let result = await p;
console.log("Result: "+ result + " (should be 10)");
}

class Storage {
constructor(databaseName) {
this._databaseName = databaseName;
this._database = null;
}

open() {
const req = window.indexedDB.open(this._databaseName);
const self = this;
req.onupgradeneeded = function(ev) {
const db = ev.target.result;
const oldVersion = ev.oldVersion;
self._createStores(db, oldVersion);
};
return reqAsPromise(req).then(function() {
self._database = req.result;
});
}

openTxn(mode, storeName) {
const txn = this._database.transaction([storeName], mode);
txn.addEventListener("complete", () => {
console.info(`transaction ${mode} for ${storeName} completed`);
});
txn.addEventListener("abort", e => {
console.warn(`transaction ${mode} for ${storeName} aborted`, e.target.error);
});
return txn;
}

_createStores(db) {
db.createObjectStore("foos", {keyPath: "id"});
}
}

async function getAll(store, depth = 0) {
if (depth < 15) {
return await getAll(store, depth + 1);
}
const request = store.openCursor();
const results = [];
return await new Promise(function(resolve, reject) {
request.onsuccess = function(event) {
const cursor = event.target.result;
if(cursor) {
results.push(cursor.value);
cursor.continue();
} else {
resolve(results);
Promise.flushQueue && Promise.flushQueue();
}
};
request.onerror = function(e) {
reject(new Error("IDB request failed: " + e.target.error.message));
Promise.flushQueue && Promise.flushQueue();
};
});
}

async function main() {
try {
let storage = new Storage("idb-promises");
await storage.open();
//await reqAsPromise(storage.openTxn("readwrite", "foos").objectStore("foos").clear());

for (let i = 0; i < 10; i += 1) {
storage.openTxn("readonly", "foos").objectStore("foos").get(5);
//console.log("from readtxn", await reqAsPromise(storage.openTxn("readonly", "foos").objectStore("foos").get(5)));
const txn = storage.openTxn("readwrite", "foos");
const store = txn.objectStore("foos");
console.log("writing the foos");
store.put({id: 5, name: "foo"});
store.put({id: 6, name: "bar"});
store.put({id: 7, name: "bazzz"});
await delay();
console.log("reading the foos");
console.log("5", await reqAsPromise(store.get(5)));
console.log("6", await reqAsPromise(store.get(6)));
console.log("7", await reqAsPromise(store.get(7)));
// await txnAsPromise(txn);
}
} catch(err) {
console.error(err);
};
}
main();
</script>
</body>
</html>

16 changes: 8 additions & 8 deletions src/legacy-polyfill.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ limitations under the License.
*/

// polyfills needed for IE11
import Promise from "../lib/es6-promise/index.js";
import {checkNeedsSyncPromise} from "./matrix/storage/idb/utils.js";

if (typeof window.Promise === "undefined") {
window.Promise = Promise;
// TODO: should be awaited before opening any session in the picker
checkNeedsSyncPromise();
}
import "core-js/stable";
import "regenerator-runtime/runtime";
import "mdn-polyfills/Element.prototype.closest";
Expand All @@ -24,14 +32,6 @@ import "mdn-polyfills/Element.prototype.closest";
// it will also include the file supporting *all* the encodings,
// weighing a good extra 500kb :-(
import "text-encoding";
import {checkNeedsSyncPromise} from "./matrix/storage/idb/utils.js";
import Promise from "../lib/es6-promise/index.js";

if (typeof window.Promise === "undefined") {
window.Promise = Promise;
// TODO: should be awaited before opening any session in the picker
checkNeedsSyncPromise();
}

// TODO: contribute this to mdn-polyfills
if (!Element.prototype.remove) {
Expand Down
Loading