Retry after conflict in history data adapter

RESOLVED FIXED in FxOS-S11 (13Nov)

Status

Firefox OS
Sync
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: michielbdejong, Assigned: michielbdejong)

Tracking

unspecified
FxOS-S11 (13Nov)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → mbdejong
Blocks: 1195647
Summary: Retry after conflict → Retry after conflict in history data adapter
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
Same issue exists in bookmarks adapter, I'll add unit tests and fix in both.
(Assignee)

Updated

3 years ago
Target Milestone: --- → FxOS-S9 (16Oct)
Target Milestone: FxOS-S9 (16Oct) → FxOS-S11 (13Nov)
(Assignee)

Comment 6

3 years ago
An easy way to deal with DataStore race conditions, is to reject the sync run altogether, see bug 1215470.
Priority: -- → P3
Created attachment 8684211 [details] [review]
[gaia] michielbdejong:1214189-datastore-conflicts > mozilla-b2g:master
(Assignee)

Updated

3 years ago
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+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1215470
(Assignee)

Comment 10

3 years ago
Oh right, no, better to do it now for bookmarks as well. I'll add it within this bug and re-submit.
(Assignee)

Comment 11

3 years ago
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+
(Assignee)

Comment 13

3 years ago
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)
(Assignee)

Comment 15

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