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

Degrade IndexedDBStore back to memory only on failure #884

Merged
merged 5 commits into from
Apr 5, 2019
Merged

Degrade IndexedDBStore back to memory only on failure #884

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ba7e85
Ensure we have crypto before accessing it in `Room` model
jryans Apr 3, 2019
dd00735
Degrade `IndexedDBStore` back to memory only on failure
jryans Apr 4, 2019
8a2f84b
Emit event when `IndexedDBStore` degrades
jryans Apr 4, 2019
3eb0c53
Clarify why it's safe to change store types on demand
jryans Apr 4, 2019
389fcfa
Ensure IDB store maintains all memory state
jryans Apr 4, 2019
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: 3 additions & 0 deletions spec/unit/room.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,9 @@ describe("Room", function() {
// events should already be MatrixEvents
return function(event) {return event;};
},
isCryptoEnabled() {
return true;
},
isRoomEncrypted: function() {
return false;
},
Expand Down
2 changes: 1 addition & 1 deletion src/models/room.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ Room.prototype.loadMembersIfNeeded = function() {
const inMemoryUpdate = this._loadMembers().then((result) => {
this.currentState.setOutOfBandMembers(result.memberEvents);
// now the members are loaded, start to track the e2e devices if needed
if (this._client.isRoomEncrypted(this.roomId)) {
if (this._client.isCryptoEnabled() && this._client.isRoomEncrypted(this.roomId)) {
bwindels marked this conversation as resolved.
Show resolved Hide resolved
this._client._crypto.trackRoomDevices(this.roomId);
}
return result.fromServer;
Expand Down
100 changes: 75 additions & 25 deletions src/store/indexeddb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

/* eslint-disable no-invalid-this */

import Promise from 'bluebird';
import {MemoryStore} from "./memory";
import utils from "../utils";
import {EventEmitter} from 'events';
import LocalIndexedDBStoreBackend from "./indexeddb-local-backend.js";
import RemoteIndexedDBStoreBackend from "./indexeddb-remote-backend.js";
import User from "../models/user";
Expand Down Expand Up @@ -110,6 +113,7 @@ const IndexedDBStore = function IndexedDBStore(opts) {
};
};
utils.inherits(IndexedDBStore, MemoryStore);
utils.extend(IndexedDBStore.prototype, EventEmitter.prototype);

IndexedDBStore.exists = function(indexedDB, dbName) {
return LocalIndexedDBStoreBackend.exists(indexedDB, dbName);
Expand Down Expand Up @@ -146,36 +150,36 @@ IndexedDBStore.prototype.startup = function() {
* client state to where it was at the last save, or null if there
* is no saved sync data.
*/
IndexedDBStore.prototype.getSavedSync = function() {
IndexedDBStore.prototype.getSavedSync = degradable(function() {
return this.backend.getSavedSync();
};
}, "getSavedSync");

/** @return {Promise<bool>} whether or not the database was newly created in this session. */
IndexedDBStore.prototype.isNewlyCreated = function() {
IndexedDBStore.prototype.isNewlyCreated = degradable(function() {
return this.backend.isNewlyCreated();
};
}, "isNewlyCreated");

/**
* @return {Promise} If there is a saved sync, the nextBatch token
* for this sync, otherwise null.
*/
IndexedDBStore.prototype.getSavedSyncToken = function() {
IndexedDBStore.prototype.getSavedSyncToken = degradable(function() {
return this.backend.getNextBatchToken();
},
}, "getSavedSyncToken"),

/**
* Delete all data from this store.
* @return {Promise} Resolves if the data was deleted from the database.
*/
IndexedDBStore.prototype.deleteAllData = function() {
IndexedDBStore.prototype.deleteAllData = degradable(function() {
MemoryStore.prototype.deleteAllData.call(this);
return this.backend.clearDatabase().then(() => {
console.log("Deleted indexeddb data.");
}, (err) => {
console.error(`Failed to delete indexeddb data: ${err}`);
throw err;
});
};
});

/**
* Whether this store would like to save its data
Expand Down Expand Up @@ -203,7 +207,7 @@ IndexedDBStore.prototype.save = function() {
return Promise.resolve();
};

IndexedDBStore.prototype._reallySave = function() {
IndexedDBStore.prototype._reallySave = degradable(function() {
this._syncTs = Date.now(); // set now to guard against multi-writes

// work out changed users (this doesn't handle deletions but you
Expand All @@ -219,14 +223,12 @@ IndexedDBStore.prototype._reallySave = function() {
this._userModifiedMap[u.userId] = u.getLastModifiedTime();
}

return this.backend.syncToDatabase(userTuples).catch((err) => {
console.error("sync fail:", err);
});
};
return this.backend.syncToDatabase(userTuples);
});

IndexedDBStore.prototype.setSyncData = function(syncData) {
IndexedDBStore.prototype.setSyncData = degradable(function(syncData) {
return this.backend.setSyncData(syncData);
};
}, "setSyncData");

/**
* Returns the out-of-band membership events for this room that
Expand All @@ -235,9 +237,9 @@ IndexedDBStore.prototype.setSyncData = function(syncData) {
* @returns {event[]} the events, potentially an empty array if OOB loading didn't yield any new members
* @returns {null} in case the members for this room haven't been stored yet
*/
IndexedDBStore.prototype.getOutOfBandMembers = function(roomId) {
IndexedDBStore.prototype.getOutOfBandMembers = degradable(function(roomId) {
return this.backend.getOutOfBandMembers(roomId);
};
}, "getOutOfBandMembers");

/**
* Stores the out-of-band membership events for this room. Note that
Expand All @@ -247,20 +249,68 @@ IndexedDBStore.prototype.getOutOfBandMembers = function(roomId) {
* @param {event[]} membershipEvents the membership events to store
* @returns {Promise} when all members have been stored
*/
IndexedDBStore.prototype.setOutOfBandMembers = function(roomId, membershipEvents) {
IndexedDBStore.prototype.setOutOfBandMembers = degradable(function(
roomId,
membershipEvents,
) {
return this.backend.setOutOfBandMembers(roomId, membershipEvents);
};
}, "setOutOfBandMembers");

IndexedDBStore.prototype.clearOutOfBandMembers = function(roomId) {
IndexedDBStore.prototype.clearOutOfBandMembers = degradable(function(roomId) {
return this.backend.clearOutOfBandMembers(roomId);
};
}, "clearOutOfBandMembers");

IndexedDBStore.prototype.getClientOptions = function() {
IndexedDBStore.prototype.getClientOptions = degradable(function() {
return this.backend.getClientOptions();
};
}, "getClientOptions");

IndexedDBStore.prototype.storeClientOptions = function(options) {
IndexedDBStore.prototype.storeClientOptions = degradable(function(options) {
return this.backend.storeClientOptions(options);
};
}, "storeClientOptions");

module.exports.IndexedDBStore = IndexedDBStore;

/**
* All member functions of `IndexedDBStore` that access the backend use this wrapper to
* watch for failures after initial store startup, including `QuotaExceededError` as
* free disk space changes, etc.
*
* When IndexedDB fails via any of these paths, we degrade this back to a `MemoryStore`
* in place so that the current operation and all future ones are in-memory only.
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here is that the next calls to the store still go to func, e.g. the idb store method and encounter a closed db, fail, and end up in the catch below degrading to the memory store?

I suppose that would work, but seems a bit convoluted.

Just an idea, but have you considered using a Proxy (seems well supported) as a meta store to switch between two implementations of the store if one fails?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first time any of the IDB methods marked degradable fails, we'll enter the catch block that one time and revert back to being just a MemoryStore.

Not sure if this is clear from the review, but IndexedDBStore already extends from MemoryStore, so it's not like we're totally transforming into some other unrelated object. We're basically "peeling off" the child layer of IndexedDBStore and becoming the parent MemoryStore only.

Future method calls to the IDB store if it degrades will then only run code from MemoryStore. The state of the object is already set up for this to work since it extends from MemoryStore.

I've gotten burned by Proxy adding perf overhead in the past, so it didn't come to mind until now... It could work, but I am not sure it would really that much more elegant?

const MetaStore = new Proxy(idbStoreInstance, {
    get(target, prop, receiver) {
         // If `prop` is on the list of methods to degrade, 
         // wrap that in another proxy...
         if (prop != "storeClientOptions" && etc.) {
             return target[prop];
         }
         return new Proxy(target[prop], {
              apply(target, thisArg, argList) {
                   try {
                       // Call the normal path
                       return target.call(thisArg, ...argList);
                   } catch (e) {
                       // Instead maybe delete and fallback
                       return MemoryStore.prototype[prop].call(...)
                   }
              },
         });
    },
});

Probably we'd need to sprinkle some async / await into the above as well... The "proxies on proxies" usually makes me avoid it.

Are there parts of the current approach I could improve through comments, etc.?

Copy link
Contributor

@bwindels bwindels Apr 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right, I see now. Sorry for being slow on this.

One more thing then: the only method in IndexedDBStore that seems to forward the call to MemoryStore seems to be deleteAllData, so the memory store state would be empty for the part that the idb store overrides? The first example I can think of would be getSavedSyncToken, but feels like there would be more code that calls the store and assume once the app is syncing, the store is not empty? Maybe I'm missing something and the memory store is being updated from somewhere else...

W.r.t. commenting, maybe just briefly mentioning above Object.setPrototypeOf(this, MemoryStore.prototype); that we're changing the class of this instance to the parent class so future calls get permanently redirected would have helped understand the code faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, after looking at the store code again, I still can't spot where we're updating the memory store from the indexeddbstore mutators. But it doesn't seem like it matters as all the read methods overriden (like you mention) in indexeddbstore only seem to be called during startup, initial sync, ... so they shouldn't be called again while the app is running.

So I guess this is fine. Maybe adjust your comment where it says that the mutators update the memory state if you agree with the above assessment, as it could be a bit misleading...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a comment to explain why it should be safe to fallback to the parent store type.

IndexedDBStore actually leaves many of the methods from MemoryStore as-is without overriding them at all. For basic sync data like rooms and users, the memory state is used to answer things like getRoom even in the IDB store. When saving, the IDB calls syncToDatabase on the backend which uses the sync accumulator to save and totally ignores the data in the store itself.

You asked about getSavedSyncToken. This one is only used on startup when loading a sync from IDB. Since we are degrading to a memory store, this would return null after degrading, which seems fine. Let's say we managed to read an old cached sync from IDB, but then it degrades at some later time. SyncApi.prototype._syncFromCache is given the saved sync data, which calls _processSyncResponse which write to the in-memory part of the store via storeUser etc. The in-memory side of IndexedDBStore is always being updated even when IDB fully operational. If IDB fails later, we'll just continue with this state we have, so the store won't be empty.

However, the extra methods for things like OOB members, etc. don't currently maintain memory state. Let's examine all the mutator methods:

Mutator Methods IDB maintains memory state?
setSyncToken Yes (not overridden in IDB)
storeGroup Yes (not overridden in IDB)
storeRoom Yes (not overridden in IDB)
removeRoom Yes (not overridden in IDB)
storeUser Yes (not overridden in IDB)
storeEvents Yes (not overridden in IDB)
storeFilter Yes (not overridden in IDB)
setFilterIdByName Yes (not overridden in IDB)
storeAccountDataEvents Yes (not overridden in IDB)
setSyncData Yes (no memory state for this)
setOutOfBandMembers No, needs to call memory also
clearOutOfBandMembers No, needs to call memory also
storeClientOptions No, needs to call memory also

So, it's good that we examined each method here, as it reveals a few to fix! (clearOutOfBandMembers was I guess never implemented for MemoryStore, so that's an extra bug I guess).

I have now fixed these new bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for your detailed look! I must indeed have forgotten to implement clearOutOfBandMembers for MemoryStore when working on LL.

Looks good now!

*
* @param {Function} func The degradable work to do.
* @param {String} fallback The method name for fallback.
* @returns {Function} A wrapped member function.
*/
function degradable(func, fallback) {
return async function(...args) {
try {
return await func.call(this, ...args);
} catch (e) {
console.error("IndexedDBStore failure, degrading to MemoryStore", e);
this.emit("degraded", e);
try {
// We try to delete IndexedDB after degrading since this store is only a
// cache (the app will still function correctly without the data).
// It's possible that deleting repair IndexedDB for the next app load,
// potenially by making a little more space available.
console.log("IndexedDBStore trying to delete degraded data");
await this.backend.clearDatabase();
console.log("IndexedDBStore delete after degrading succeeeded");
} catch (e) {
console.warn("IndexedDBStore delete after degrading failed", e);
}
// Degrade the store from being an instance of `IndexedDBStore` to instead be
// an instance of `MemoryStore` so that future API calls use the memory path
// directly and skip IndexedDB entirely. This should be safe as
// `IndexedDBStore` already extends from `MemoryStore`, so we are making the
// store become its parent type in a way. The mutator methods of
// `IndexedDBStore` also maintain the state that `MemoryStore` uses (many are
// not overridden at all).
Object.setPrototypeOf(this, MemoryStore.prototype);
if (fallback) {
return await MemoryStore.prototype[fallback].call(this, ...args);
}
}
};
}