[email/backend] Do not invoke operation callbacks until after any associated saves have safely hit the disk

RESOLVED FIXED

Status

Firefox OS
Gaia::E-Mail
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: asuth, Assigned: asuth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
As part of a recent save cleanup as part of a review pass on bug 921050 comment 32 I changed us to not say a sync had completed until the save had hit the disk.  I also changed operations to not advance to the next operation until the prior operation's save had hit the disk.  However, I left it so that the callback would still be issued before the save hit the disk.

This surfaced as a problem when :awiss was running the test_mime cases since getBody({ withBodyReps }) without a protective expect_runOp was letting the test step complete before the saveAccountState_end was issued.

I think we should change it so that operation-based callbacks only happen after any save completes.  The rationale is basically:

- Any logic that shuts down the app (via window.close) or effectively shuts down the app (releasing a wake-lock, for example) is going to do absolutely the wrong thing if we issue the callback before the save completes.

- These callbacks are not actually intended to be used for UI feedback in cases were the latency matters.  For example, modifying flags on a message does not use these callbacks!  In the cases where they are used, they either arguably shouldn't really be used because they're information-lossy hacks (downloads use this and so if you leave the reader and go back, you no longer know what's going on with the download), or we probably don't want to mislead the user about the database save state.  Like if they're waiting to power-down the phone.  (Ideally they'd never be fast enough for that, but the principle seems sound.)

- It's a pretty clear win for unit tests.  Especially when we do our test refactoring, it's great if we don't need to be using expect_runOp for correctness when we don't really care about the stuff happening under the hood.

- From a performance perspective, this doesn't actually affect aggregate performance at all.  The next operation will still run at the same time, the callback just is not being issued at a dangerous time.
(Assignee)

Comment 1

4 years ago
Created attachment 8457746 [details] [review]
perform callbacks after saves,  plus some test cleanup fallout, details in commit/PR
Attachment #8457746 - Flags: review?(m)
Attachment #8457746 - Flags: review?(m) → review+
You need to log in before you can comment on or make changes to this bug.