1068054 - Figure out the love story between Sync and an async bookmarking API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

I guess thi bug will be a lot of fun.

We need to figure how to proceed here, in the next 2 weeks we will introduce a new async bookmarking API... from there we will start converting internal consumers and it would be really sad to have race conditions due to some consumers using the new and some using the old API (provided we will have this issue with add-ons regardless).

So, what can we do in the Sync world to adapt the current bookmarks engine to an async API? Do we have resources to do that? where can we find them?

I'm calling this a 13 points cause it may be really really fancy.

Comment 1

[Marco Bonardo [:mak]]

let's start the guessing dance.

Comment 2

[Marco Bonardo [:mak]]

and while here, please let me know any special requirement from Sync regarding a new API. If it needs special actions this is the right time to add those.

Comment 3

[Richard Newman [:rnewman]]

In short:

1. Any synchronous calls that are going away (e.g., insertBookmark?) either need to be wrapped in spinning, or the call site needs to be adjusted to be async and spinning introduced further up the stack. E.g., see what we did with the PlacesBackups.create call in _syncStartup.

2. Replace all of the async SQL queries in bookmarks.js with API calls. These are already wrapped in spinning.

The important point is that the various Sync engine API calls -- e.g., applyIncoming -- are synchronous; there are assumptions encoded here, such as "the previous record has already been inserted and various maps updated by the time we apply the next record", or "all incoming records have been applied before we begin uploading outgoing records".

Whatever goes on within those stages can be async.

Mark has a speculative patch in Bug 1007448 that I haven't had time to review. That should make the entirety of Sync callback driven, and probably introduce a huge pile of edge-case bugs :D

Comment 4

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #3)
> 1. Any synchronous calls that are going away (e.g., insertBookmark?) either
> need to be wrapped in spinning, or the call site needs to be adjusted to be
> async and spinning introduced further up the stack. E.g., see what we did
> with the PlacesBackups.create call in _syncStartup.

I had such suspect. No way we can have an async Sync in "short" times  (like 1 year)? are there plans for it? I suspect it would be a priority for the perf team.

> 2. Replace all of the async SQL queries in bookmarks.js with API calls.
> These are already wrapped in spinning.

Yes, I will go through those queries and ensure the API can replace them. Even if I suspect we won't implement an API to change a GUID initially (though now you can set a guid on creation) cause it's very hard to track guid changes and ensure data safety (a guid is considered immutable).

> Mark has a speculative patch in Bug 1007448 that I haven't had time to
> review. That should make the entirety of Sync callback driven, and probably
> introduce a huge pile of edge-case bugs :D

That sounds nice, will that remove the need to spin the events loop?

Do you know if any of you might have an available iteration to convert Sync bookmarks engine to the new API from (about) the end of Semptember?

Comment 5

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #4)

> I had such suspect. No way we can have an async Sync in "short" times  (like
> 1 year)? are there plans for it? I suspect it would be a priority for the
> perf team.

That's really up to the desktop team. As far as I know -- as for the last two years -- there's nobody assigned exclusively to work on Sync internals, and nobody championing work into the backlog.

Recent discussions with folks around me have taken the direction that if we want something, and it's not already something Sync does, we'll build a new service for it. Certainly nothing we fix would fix Sync's data model, syncing logic, or other fundamental flaws.


> Even if I suspect we won't implement an API to change a GUID initially
> (though now you can set a guid on creation) cause it's very hard to track
> guid changes and ensure data safety (a guid is considered immutable).

Bug 909256, Bug 1012597, right?


> That sounds nice, will that remove the need to spin the events loop?

It should.


> Do you know if any of you might have an available iteration to convert Sync
> bookmarks engine to the new API from (about) the end of Semptember?

I have (as for the last two years) approximately zero cycles available for desktop Sync -- I have an infinite amount of work to do for other platforms.

Mark or Tim might have time available to do the work. I can make time to review (or at least superreview).

Comment 6

[Marco Bonardo [:mak]]

Still quite some calls:
http://searchfox.org/mozilla-central/search?q=PlacesUtils%5C.bookmarks.*(Bookmark%7CItem%7CSeparator%7CFolder%7CKeyword%7CURI).*%5C(&case=false&regexp=true&path=sync

Comment 7

[Edouard Oger [:eoger]]

Kinda related (to convert bookmarks.js).

Comment 8

[Marco Bonardo [:mak]]

Posting a better link to old bookmarks API usage in Services.

Comment 9

[Marco Bonardo [:mak]]

Is there an ETA to remove at least the non-test usage?

Everything needed to remove the old APIs is tracked and in progress for 57, apart from tags that are being worked on by a volunteer, and the addons-sdk that should go away by itself. Converting all the tests may take a little bit more time, and thus any used old API will stay around a bit more just for those, hopefully to be converted by the end of the 58 cycle (provided there are no big surprises!).

Comment 10

[Marco Bonardo [:mak]]

Note that in non-convertible cases, we'll keep the old API around longer. For example we have one case with D&D that we can't easily convert, so feel free to point those out. I'm fine keeping things that are very hard to convert around until we find a solution.

Comment 11

[Marco Bonardo [:mak]]

Looks like all the usage is inside _ensureMobileQuery that is invoked by async _syncFinish()... so it *should* be await-able?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Marco Bonardo [::mak] from comment #11)
> Looks like all the usage is inside _ensureMobileQuery that is invoked by
> async _syncFinish()... so it *should* be await-able?

Yep, that's not a problem. I opened 3 dependent bugs, and I hope we can get at least bug 1383621 marked as P1 or P2 in triage this week.

Comment 13

[Lina Butler [:lina]]

This should be fixed now. The only remaining uses of the sync API are in the tracker tests, which we can remove when we remove the API.