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.

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

Jump to bottom

Dexie.Observable: startObserving function: remove read-only query in order to avoid TransactionInactiveError #1138

Merged
merged 11 commits into from
Oct 14, 2020
Merged

Dexie.Observable: startObserving function: remove read-only query in order to avoid TransactionInactiveError #1138

Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ec3e03
Do not re-use current transaction when observation begins
jayaddison Oct 6, 2020
0035e27
Revert "Do not re-use current transaction when observation begins"
jayaddison Oct 7, 2020
5237d33
startObserving: serialize database operations, and ignore current tra…
jayaddison Oct 7, 2020
57b1d07
Revert "startObserving: serialize database operations, and ignore cur…
jayaddison Oct 8, 2020
f4de913
Alternative approach: place inner write operations within nested tran…
jayaddison Oct 8, 2020
bbd73a0
Use a single, ordered pass over the syncNodes table to modify and set…
jayaddison Oct 8, 2020
76e3c76
Nit: adjust code formatting
jayaddison Oct 8, 2020
b645fd1
Add/update explanatory comments
jayaddison Oct 8, 2020
7fc0c88
Assign updated master value to local sync node variable in addition t…
jayaddison Oct 8, 2020
0825fec
Add safety check (with associated TODO)
jayaddison Oct 8, 2020
8c74e39
Add explanatory comment and remove TODO message
jayaddison Oct 8, 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
26 changes: 15 additions & 11 deletions addons/Dexie.Observable/src/Dexie.Observable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,19 +254,23 @@ function Observable(db) {
// Take over mastership
mySyncNode.node.isMaster = 1;
currentMaster.isMaster = 0;
return db._syncNodes.put(currentMaster);
db.transaction('rw!', '_syncNodes', () => {
return db._syncNodes.put(currentMaster);
});
}
}).then(()=>{
// Add our node to DB and start subscribing to events
return db._syncNodes.add(mySyncNode.node).then(function() {
Observable.on('latestRevisionIncremented', onLatestRevisionIncremented); // Wakeup when a new revision is available.
Observable.on('beforeunload', onBeforeUnload);
Observable.on('suicideNurseCall', onSuicide);
Observable.on('intercomm', onIntercomm);
// Start polling for changes and do cleanups:
pollHandle = setTimeout(poll, LOCAL_POLL);
// Start heartbeat
heartbeatHandle = setTimeout(heartbeat, HEARTBEAT_INTERVAL);
db.transaction('rw!', '_syncNodes', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 'works', but needs a bit of scrutiny as to whether it retains atomicity:

Ideally some test coverage to check & prove atomicity would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another related thought: this is currently essentially performing a get-and-set within a transaction to ensure atomicity.

Would it be possible instead to use a single write operation (Table.modify or similar) to perform the required updates?

That way there would be no read-only query required within the transaction, and the transaction would not risk becoming inactive as a result of that query completing before any writes occur.

Our requirements are to:

  • Set the current node's isMaster flag when no record with isMaster and an up-to-date heartbeat timestamp is present
  • Unset the isMaster flag on a record if the heartbeat timestamp is out-of-date
  • Write the current node record into the table

The challenge is that the value we write for the current node record depends on the contents of a potentially existing (or potentially nonexistent) record with isMaster set and potentially out-of-date heartbeat timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like the isMaster field is an indexable type (ref: definition as Number), so perhaps it would be possible to use db._syncNodes.orderBy(...).modify(...) to retrieve existing syncnodes in order, maintain an external variable to store the 'current node should become master' state, and then use a function argument to modify to write the expected object record(s).

// Add our node to DB and start subscribing to events
return db._syncNodes.add(mySyncNode.node).then(function() {
Observable.on('latestRevisionIncremented', onLatestRevisionIncremented); // Wakeup when a new revision is available.
Observable.on('beforeunload', onBeforeUnload);
Observable.on('suicideNurseCall', onSuicide);
Observable.on('intercomm', onIntercomm);
// Start polling for changes and do cleanups:
pollHandle = setTimeout(poll, LOCAL_POLL);
// Start heartbeat
heartbeatHandle = setTimeout(heartbeat, HEARTBEAT_INTERVAL);
});
});
});
}).then(function () {
Expand Down