Skip to content

Commit

Permalink
clean up save behaviour, tests in general, fix outbox tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@asutherland
asutherland committed Jul 11, 2014
1 parent a7272e3 commit 59f9072
Show file tree
Hide file tree
Showing 25 changed files with 991 additions and 704 deletions.
7 changes: 6 additions & 1 deletion data/lib/mailapi/accountmixins.js
View file
Expand Up @@ -214,6 +214,8 @@ exports.saveAccountState = function(reuseTrans, callback, reason) {
return null;
}

this._LOG.saveAccountState_begin(reason, null);

// Indicate save is active, in case something, like
// signaling the end of a sync, needs to run after
// a save, via runAfterSaves.
Expand All @@ -233,12 +235,13 @@ exports.saveAccountState = function(reuseTrans, callback, reason) {
if (folderStuff)
perFolderStuff.push(folderStuff);
}
this._LOG.saveAccountState(reason);
var folderSaveCount = perFolderStuff.length;
var trans = this._db.saveAccountFolderStates(
this.id, this._folderInfos, perFolderStuff,
this._deadFolderIds,
function stateSaved() {
this._saveAccountStateActive = false;
this._LOG.saveAccountState_end(reason, folderSaveCount);

// NB: we used to log when the save completed, but it ended up being
// annoying to the unit tests since we don't block our actions on
Expand All @@ -251,6 +254,8 @@ exports.saveAccountState = function(reuseTrans, callback, reason) {
});
}.bind(this),
reuseTrans);
// Reduce the length of time perFolderStuff and its contents are kept alive.
perFolderStuff = null;
this._deadFolderIds = null;
return trans;
};
Expand Down
6 changes: 3 additions & 3 deletions data/lib/mailapi/activesync/account.js
View file
Expand Up @@ -339,8 +339,8 @@ ActiveSyncAccount.prototype = {
* We are being told that a synchronization pass completed, and that we may
* want to consider persisting our state.
*/
__checkpointSyncCompleted: function() {
this.saveAccountState(null, null, 'checkpointSync');
__checkpointSyncCompleted: function(callback, betterReason) {
this.saveAccountState(null, callback, betterReason || 'checkpointSync');
},

shutdown: function asa_shutdown(callback) {
Expand Down Expand Up @@ -962,7 +962,6 @@ var LOGFAB = exports.LOGFAB = $log.register($module, {
createFolder: {},
deleteFolder: {},
recreateFolder: { id: false },
saveAccountState: { reason: false },
/**
* XXX: this is really an error/warning, but to make the logging less
* confusing, treat it as an event.
Expand All @@ -971,6 +970,7 @@ var LOGFAB = exports.LOGFAB = $log.register($module, {
},
asyncJobs: {
runOp: { mode: true, type: true, error: false, op: false },
saveAccountState: { reason: true, folderSaveCount: true },
},
errors: {
opError: { mode: false, type: false, ex: $log.EXCEPTION },
Expand Down
53 changes: 30 additions & 23 deletions data/lib/mailapi/activesync/folder.js
View file
Expand Up @@ -1423,30 +1423,37 @@ ActiveSyncFolderSyncer.prototype = {
// Expand the accuracy range to cover everybody.
if (!err)
storage.markSyncedToDawnOfTime();
// Always save state, although as an optimization, we could avoid saving state
// if we were sure that our state with the server did not advance.
this._account.__checkpointSyncCompleted();

if (err) {
doneCallback(err);
return;
}

if (initialSync) {
storage._curSyncSlice.ignoreHeaders = false;
storage._curSyncSlice.waitingOnData = 'db';

storage.getMessagesInImapDateRange(
0, null, $sync.INITIAL_FILL_SIZE, $sync.INITIAL_FILL_SIZE,
// Don't trigger a refresh; we just synced. Accordingly, releaseMutex can
// be null.
storage.onFetchDBHeaders.bind(storage, storage._curSyncSlice, false,
doneCallback, null)
);
}
else {
doneCallback(err);
}
// Always save state, although as an optimization, we could avoid saving
// state if we were sure that our state with the server did not advance.
// Do not call our callback until the save has completed.
this._account.__checkpointSyncCompleted(function() {
if (err) {
doneCallback(err);
}
else if (initialSync) {
storage._curSyncSlice.ignoreHeaders = false;
storage._curSyncSlice.waitingOnData = 'db';

// TODO: We could potentially shave some latency by doing the DB fetch
// but deferring the doneCallback until the checkpoint has notified.
// I'm copping out on this right now because there may be some nuances
// in there that I would like to think about more and this is also not
// a major slowdown concern. We're already slow here and the more
// important thing for us to do would just be to trigger the initial
// sync much earlier in the UI process to save even more time.
storage.getMessagesInImapDateRange(
0, null, $sync.INITIAL_FILL_SIZE, $sync.INITIAL_FILL_SIZE,
// Don't trigger a refresh; we just synced. Accordingly,
// releaseMutex can be null.
storage.onFetchDBHeaders.bind(storage, storage._curSyncSlice, false,
doneCallback, null)
);
}
else {
doneCallback(err);
}
});
},

allConsumersDead: function() {
Expand Down
6 changes: 3 additions & 3 deletions data/lib/mailapi/composite/incoming.js
View file
Expand Up @@ -248,8 +248,8 @@ CompositeIncomingAccount.prototype = {
* We are being told that a synchronization pass completed, and that we may
* want to consider persisting our state.
*/
__checkpointSyncCompleted: function(callback) {
this.saveAccountState(null, callback, 'checkpointSync');
__checkpointSyncCompleted: function(callback, betterReason) {
this.saveAccountState(null, callback, betterReason || 'checkpointSync');
},

/**
Expand Down Expand Up @@ -384,7 +384,6 @@ exports.LOGFAB_DEFINITION = {
unknownDeadConnection: {},
connectionMismatch: {},

saveAccountState: { reason: false },
/**
* XXX: this is really an error/warning, but to make the logging less
* confusing, treat it as an event.
Expand Down Expand Up @@ -414,6 +413,7 @@ exports.LOGFAB_DEFINITION = {
asyncJobs: {
checkAccount: { err: null },
runOp: { mode: true, type: true, error: false, op: false },
saveAccountState: { reason: true, folderSaveCount: true },
},
TEST_ONLY_asyncJobs: {
},
Expand Down
31 changes: 17 additions & 14 deletions data/lib/mailapi/imap/folder.js
View file
Expand Up @@ -1114,21 +1114,24 @@ ImapFolderSyncer.prototype = {
//
this._syncSlice.desiredHeaders = this._syncSlice.headers.length;

if (this._curSyncDoneCallback)
this._curSyncDoneCallback(err);

// Save our state even if there was an error because we may have accumulated
// some partial state.
this._account.__checkpointSyncCompleted();

this._syncSlice = null;
this._curSyncAccuracyStamp = null;
this._curSyncDir = null;
this._nextSyncAnchorTS = null;
this._syncThroughTS = null;
this._curSyncDayStep = null;
this._curSyncDoNotGrowBoundary = null;
this._curSyncDoneCallback = null;
// some partial state. Additionally, don't *actually* complete until the
// save has hit the disk. This is beneficial for both tests and cronsync
// which has been trying to shut us down in a race with this save
// triggering.
this._account.__checkpointSyncCompleted(function () {
if (this._curSyncDoneCallback)
this._curSyncDoneCallback(err);

this._syncSlice = null;
this._curSyncAccuracyStamp = null;
this._curSyncDir = null;
this._nextSyncAnchorTS = null;
this._syncThroughTS = null;
this._curSyncDayStep = null;
this._curSyncDoNotGrowBoundary = null;
this._curSyncDoneCallback = null;
}.bind(this));
},

/**
Expand Down
13 changes: 8 additions & 5 deletions data/lib/mailapi/jobs/outbox.js
View file
Expand Up @@ -55,6 +55,13 @@ define(function(require) {
};
}

// Figure out if this is the last message to consider sending in the
// outbox. (We are moving from newest to oldest, so this is the last one
// if it is the oldest. We need to figure this out before the send
// process completes since we will delete the header once it's all sent.)
var moreExpected = !storage.headerIsOldestKnown(header.date,
header.id);

if (!header.sendStatus) {
header.sendStatus = {};
}
Expand All @@ -66,12 +73,8 @@ define(function(require) {
return constructComposer(account, storage, header, wakeLock)
.then(sendMessage.bind(null, account, storage, emitNotifications))
.then(function(header) {
// Figure out if this was the last message in the outbox.
// If `storage` is empty, getOldestMessageTimestamp
// returns zero.
var oldestDate = storage.getOldestMessageTimestamp();
return {
moreExpected: oldestDate > 0 && header.date !== oldestDate,
moreExpected: moreExpected,
messageNamer: {
suid: header.suid,
date: header.date
Expand Down
78 changes: 50 additions & 28 deletions data/lib/mailapi/mailuniverse.js
View file
Expand Up @@ -935,13 +935,22 @@ MailUniverse.prototype = {
/**
* Write the current state of the universe to the database.
*/
saveUniverseState: function() {
saveUniverseState: function(callback) {
var curTrans = null;
var latch = $allback.latch();

this._LOG.saveUniverseState_begin();
for (var iAcct = 0; iAcct < this.accounts.length; iAcct++) {
var account = this.accounts[iAcct];
curTrans = account.saveAccountState(curTrans, null, 'saveUniverse');
curTrans = account.saveAccountState(curTrans, latch.defer(account.id),
'saveUniverse');
}
latch.then(function() {
this._LOG.saveUniverseState_end();
if (callback) {
callback();
};
}.bind(this));
},

/**
Expand Down Expand Up @@ -1138,6 +1147,9 @@ MailUniverse.prototype = {
completeOp = true;
break;
}

// Do not save if this was an error.
accountSaveSuggested = false;
}
else {
switch (op.localStatus) {
Expand All @@ -1156,11 +1168,6 @@ MailUniverse.prototype = {
}
break;
}

// This is a suggestion; in the event of high-throughput on operations,
// we probably don't want to save the account every tick, etc.
if (accountSaveSuggested)
account.saveAccountState(null, null, 'localOp');
}

if (removeFromServerQueue) {
Expand All @@ -1184,25 +1191,13 @@ MailUniverse.prototype = {
}
}

// We must hold off on freeing up queue.active until after we have
// completed processing and called the callback, because the
// callback may attempt to schedule additional jobs. By setting
// queues.active to false here, we know that it's still our
// responsibility to schedule the next job.
queues.active = false;

if (localQueue.length) {
op = localQueue[0];
this._dispatchLocalOpForAccount(account, op);
}
else if (serverQueue.length && this.online && account.enabled) {
op = serverQueue[0];
this._dispatchServerOpForAccount(account, op);
}
else if (this._opCompletionListenersByAccount[account.id]) {
this._opCompletionListenersByAccount[account.id](account);
this._opCompletionListenersByAccount[account.id] = null;
if (accountSaveSuggested) {
account.saveAccountState(null, this._startNextOp.bind(this, account),
'localOp');
return;
}

this._startNextOp(account);
},

/**
Expand Down Expand Up @@ -1407,10 +1402,33 @@ MailUniverse.prototype = {
// we probably don't want to save the account every tick, etc.
if (accountSaveSuggested) {
account._saveAccountIsImminent = false;
account.saveAccountState(null, null, 'serverOp');
account.saveAccountState(null, this._startNextOp.bind(this, account),
'serverOp');
return;
}
}

this._startNextOp(account)
},

/**
* Shared code for _localOpCompleted and _serverOpCompleted to figure out what
* to do next *after* any account save has completed. It used to be that we
* would trigger saves without waiting for them to complete with the theory
* that this would allow us to generally be more efficient without losing
* correctness since the IndexedDB transaction model is strong and takes care
* of data dependency issues for us. However, for both testing purposes and
* with some new concerns over correctness issues, it's now making sense to
* wait on the transaction to commit. There are potentially some memory-use
* wins from waiting for the transaction to complete, especially if we
* imagine some particularly pathological situations.
*/
_startNextOp: function(account, queues) {
var queues = this._opsByAccount[account.id],
serverQueue = queues.server,
localQueue = queues.local;
var op;

// We must hold off on freeing up queue.active until after we have
// completed processing and called the callback, just as we do in
// _localOpCompleted. This allows `callback` to safely schedule
Expand Down Expand Up @@ -1510,8 +1528,9 @@ MailUniverse.prototype = {

waitForAccountOps: function(account, callback) {
var queues = this._opsByAccount[account.id];
if (queues.local.length === 0 &&
(queues.server.length === 0 || !this.online || !account.enabled))
if (!queues.active &&
queues.local.length === 0 &&
(queues.server.length === 0 || !this.online || !account.enabled))
callback();
else
this._opCompletionListenersByAccount[account.id] = callback;
Expand Down Expand Up @@ -2174,6 +2193,9 @@ var LOGFAB = exports.LOGFAB = $log.register($module, {
configLoaded: { config: false, accounts: false },
createAccount: { name: false },
},
asyncJobs: {
saveUniverseState: {}
},
errors: {
badAccountType: { type: true },
opCallbackErr: { type: false },
Expand Down

0 comments on commit 59f9072

Please sign in to comment.