Closed Bug 1214189 Opened 9 years ago Closed 9 years ago

Retry after conflict in history data adapter

Categories

(Firefox OS Graveyard :: Sync, defect, P3)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S11 (13Nov)

People

(Reporter: mbdejong, Assigned: mbdejong)

References

Details

Attachments

(1 file)

It seems to me that a DataStore.put with revisionId parameter can fail. When this happens in https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L127 we should probably repeat the get->merge->put cycle, until it succeeds.
Assignee: nobody → mbdejong
Blocks: 1195647
Summary: Retry after conflict → Retry after conflict in history data adapter
Sean, do you agree?
Flags: needinfo?(selee)
Hey Michiel,

Thanks for finding this. :)
Could you paste the log message or the steps to reproduce?
Flags: needinfo?(selee)
In addition, I didn't get why DataStore.put with revisionId parameter can fail. So I am looking forward to more information. :P
I don't have steps to reproduce, just ran into this question while reading the code.

The docs [1] say DataStore.put can "be aborted", so I guess this will look like a Promise rejection, but I'm not sure. Will try it out and propose a patch!

[1] https://developer.mozilla.org/en-US/docs/Web/API/DataStore/put#Parameters
Same issue exists in bookmarks adapter, I'll add unit tests and fix in both.
Target Milestone: --- → FxOS-S9 (16Oct)
Target Milestone: FxOS-S9 (16Oct) → FxOS-S11 (13Nov)
An easy way to deal with DataStore race conditions, is to reject the sync run altogether, see bug 1215470.
Priority: -- → P3
Attachment #8684211 - Flags: review?(ferjmoreno)
Comment on attachment 8684211 [details] [review]
[gaia] michielbdejong:1214189-datastore-conflicts > mozilla-b2g:master

LGTM, thanks.

It seems that you forgot to add the fix for bookmarks as you mentioned in comment 5. Do you still want to do it in this bug or do you want to do it in a follow up?
Attachment #8684211 - Flags: review?(ferjmoreno) → review+
Oh right, no, better to do it now for bookmarks as well. I'll add it within this bug and re-submit.
Comment on attachment 8684211 [details] [review]
[gaia] michielbdejong:1214189-datastore-conflicts > mozilla-b2g:master

Made same change for bookmarks as well.
Attachment #8684211 - Flags: review+ → review?(ferjmoreno)
Comment on attachment 8684211 [details] [review]
[gaia] michielbdejong:1214189-datastore-conflicts > mozilla-b2g:master

Thank you Michiel. LGTM, but there is a failing test.

Also, could you file a bug to move the common logic between the history and bookmarks adapter to a shared script, please?
Attachment #8684211 - Flags: review?(ferjmoreno) → review+
Thanks!

Failing tests fixed with https://github.com/mozilla-b2g/gaia/pull/33043/files#diff-6dac2f6d58d68fbc11a10804f9946d4fR178 (still waiting for TreeHerder).

Follow-up is bug 1224183.
Flags: needinfo?(ferjmoreno)
Needs rebasing
Flags: needinfo?(ferjmoreno) → needinfo?(mbdejong)
Ah sorry, rebased (waiting for TreeHerder again).
Flags: needinfo?(mbdejong) → needinfo?(ferjmoreno)
https://github.com/mozilla-b2g/gaia/commit/ee5d88665fa0f6b02afd13173f5dfa3459dee2fa
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(ferjmoreno)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: