Closed Bug 785225 Opened 7 years ago Closed 7 years ago

War on singletons

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(11 files, 5 obsolete files)

3.29 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
22.02 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
43.04 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
7.21 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
22.63 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
11.23 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
7.58 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
101.70 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
6.59 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
88.17 KB, patch
rnewman
: feedback+
Details | Diff | Splinter Review
106.53 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
Sync refactoring to use dedicated objects for settings and state (which is required for repositories and async rewrite) will require a lot of singletons to go away. Singletons (usually) are a bad programming practice anyway, so if nothing else, this shovels a lot of technical debt out of the way.

Part 1 is simple: it removes the Engines singleton from AddonReconciler. It does shift it to addons.js, but I'll deal with that later.
Attachment #654783 - Flags: review?(mconnor)
1) Store EngineManager instance in Service instance. (It is actually the singleton instance for now. We'll have to kill the Engines singleton before this can be changed.)

2) Update browser code to access EngineManager instance hung off Weave.Service. Don't export Weave.Engines.

xpcshell tests pass.
Attachment #654795 - Flags: review?(mconnor)
Pretty self-explanatory. xpcshell tests pass.
Attachment #654818 - Flags: review?(mconnor)
A number of exports from the Weave global variable are not used. No sense exporting them!
Attachment #654824 - Flags: review?(mconnor)
I goofed. Turns out there was some dynamic language magic in service.js for engine loading that was accessing Weave[engine]. I rolled a fix for that into this patch. Engine to file mapping is now in service.js.

I admit these should have been separate patches. Oh well.
Attachment #654824 - Attachment is obsolete: true
Attachment #654824 - Flags: review?(mconnor)
Attachment #654846 - Flags: review?(mconnor)
See commit message for details.
Attachment #654850 - Flags: review?(mconnor)
What doesn't belong in policies.js? SendCredentialsController, that's what! It just happened to be caught up in the net as part of all the refactoring.
Attachment #654880 - Flags: review?(rnewman)
Nuke the SyncScheduler singleton reference from StorageCredentialsController by passing a SyncScheduler to the constructor.
Attachment #654882 - Flags: review?(rnewman)
Submitted last patch too early.
Assignee: nobody → gps
Attachment #654880 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #654880 - Flags: review?(rnewman)
Attachment #654890 - Flags: review?(rnewman)
Now binding to a Service instance.
Attachment #654882 - Attachment is obsolete: true
Attachment #654882 - Flags: review?(rnewman)
Attachment #654891 - Flags: review?(rnewman)
SyncScheduler is a singleton no more.
Attachment #654897 - Flags: review?(rnewman)
See the commit message for description.
Attachment #654922 - Flags: review?(rnewman)
All xpcshell tests pass \o/
Attachment #654923 - Flags: review?(rnewman)
Attachment #654783 - Flags: review?(mconnor) → review?(rnewman)
Attachment #654795 - Flags: review?(mconnor) → review?(rnewman)
Attachment #654818 - Flags: review?(mconnor) → review?(rnewman)
Attachment #654846 - Flags: review?(mconnor) → review?(rnewman)
Attachment #654850 - Flags: review?(mconnor) → review?(rnewman)
OK, I think I'm done with this bug. So, it's time for some general comments for the reviewer.

The existing Sync code has singletons everywhere. This would be great if singletons were warranted, but they aren't.

As part of my async rewrite and general refactoring, I found that the tight coupling between all these singletons was making it extremely hard to make simple changes. The web of entanglement was absurd and simple changes involved chasing down singleton references in many files. I decided to bite the bullet and eliminate most of the singletons as a dedicated series of patches.

Now, the new world isn't perfect. Lots of the types still have things like magic setters that write preferences. So, there is no true barrier between instances. Also, there is still only 1 Service instance. And, lots of the objects that were singletons before now hang off of the Service instance. So, effectively, they are still singletons. But, at least you can instantiate other instances if you really wanted to.

I expect this work to pay dividends in the future. It is on the road to allow true separation between all type instances. Eventually, we'll be able to create a whole new "Sync" instance running on a clean slate. This will make things like tests much easier to write. It will also make the code cleaner since we don't have to reset state in a bunch of singletons every time there is a major change event. It will also allow more advanced usage patterns, such as multiple Sync services running in parallel. Although, I'm not sure if we'd ever need that.
Comment on attachment 654783 [details] [diff] [review]
Part 1: Remove Engines singleton from AddonReconciler, v1

Review of attachment 654783 [details] [diff] [review]:
-----------------------------------------------------------------

Reverse order of statements in stop; unwind in reverse order.
Attachment #654783 - Flags: review?(rnewman) → review+
Comment on attachment 654795 [details] [diff] [review]
Part 2: Reduce Engines usage in service; don't export Weave.Engines, v1

Review of attachment 654795 [details] [diff] [review]:
-----------------------------------------------------------------

This will break any of our wiki snippets that rely on Weave access to Engines. be aware.
Attachment #654795 - Flags: review?(rnewman) → review+
Comment on attachment 654818 [details] [diff] [review]
Part 3: Make ErrorHandler an instance, not a singleton, v1

Review of attachment 654818 [details] [diff] [review]:
-----------------------------------------------------------------

Nits. bind, not self. && at end of line, not start. 

Also, God, it's sad we have to do all this. But thanks for doing it! 

apologies for brief and shitty reviews, on phone. I trust your deciphering skills!
Attachment #654818 - Flags: review?(rnewman) → review+
Comment on attachment 654846 [details] [diff] [review]
Part 4: Remove unused exports from Weave; change engine loading, v2

Review of attachment 654846 [details] [diff] [review]:
-----------------------------------------------------------------

service 436: use engineName.
Attachment #654846 - Flags: review?(rnewman) → review+
Previous patches broke TPS tests. This unbusts.

I can't verify this completely because my local machine is having issues with TPS tests again. I popped all patches and it still fails. Hmmm.
Attachment #655147 - Flags: review?(rnewman)
The constructors for Engine, Store, and Tracker now enforce the presence of an engine and service. This results in a lot of new test failures which are cleaned up in the subsequent patch.
Attachment #654922 - Attachment is obsolete: true
Attachment #654922 - Flags: review?(rnewman)
Attachment #655199 - Flags: review?(rnewman)
Pass Engine and/or Service to appropriate constructors.
Attachment #654923 - Attachment is obsolete: true
Attachment #654923 - Flags: review?(rnewman)
Attachment #655201 - Flags: review?(rnewman)
Comment on attachment 654850 [details] [diff] [review]
Part 5: Reduce Clients singleton usage, v1

Review of attachment 654850 [details] [diff] [review]:
-----------------------------------------------------------------

Dummy question:

browser/components/preferences/sync.js
browser/components/preferences/in-content/sync.js

Is one of these generated from the other? Why do we have both of these?

::: browser/base/content/sync/aboutSyncTabs.js
@@ +154,5 @@
>          if (appendClient) {
>            let attrs = {
>              type: "client",
>              clientName: client.clientName,
> +            class: Weave.Service.clientsEngine.isMobile(client.id) ? "mobile" : "desktop"

This raises a small red flag to me.

You're changing a singleton access to something that isn't necessarily an engine (apart from the access to lastSync, below), to a service-linked engine instance.

In a way, this is more tightly coupled, particularly when you consider future progressions of (persistent) Service into (transient) GlobalSession.

We hit this same thing with Android Sync: how do you interact with (clients, tabs) storage outside of the context of a sync?

Arguably clients is a really special case; it's not an engine at all, it just has to pretend to be one and stick some data on the server. That's pretty much where we ended up:

https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/delegates/ClientsDataDelegate.java

… with SyncAdapter (the 'topmost' object) implementing ClientsDataDelegate. GlobalSession doesn't.

Anyway, you'll hit that in a later patch in the series, I'm sure.
Attachment #654850 - Flags: review?(rnewman) → review+
Comment on attachment 654890 [details] [diff] [review]
Part 6: Move StorageCredentialsController to jpakeclient.js, v2

Review of attachment 654890 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/tests/unit/test_sendcredentials_controller.js
@@ +1,2 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */ 

Trailing whitespace.
Attachment #654890 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #21)
> Dummy question:
> 
> browser/components/preferences/sync.js
> browser/components/preferences/in-content/sync.js
> 
> Is one of these generated from the other? Why do we have both of these?

I... don't know!

> ::: browser/base/content/sync/aboutSyncTabs.js
> @@ +154,5 @@
> >          if (appendClient) {
> >            let attrs = {
> >              type: "client",
> >              clientName: client.clientName,
> > +            class: Weave.Service.clientsEngine.isMobile(client.id) ? "mobile" : "desktop"
> 
> This raises a small red flag to me.
> 
> You're changing a singleton access to something that isn't necessarily an
> engine (apart from the access to lastSync, below), to a service-linked
> engine instance.
> 
> In a way, this is more tightly coupled, particularly when you consider
> future progressions of (persistent) Service into (transient) GlobalSession.

Tell me about it. In the current world, ClientsEngine is overloaded to be not just collection syncing but singleton like service-y behavior as well. This will need to be separated in the future.

All this will be rewritten anyway. The patches in this bug are only about facilitating that rewrite and aren't representative of the end state.
(In reply to Gregory Szorc [:gps] from comment #23)
> (In reply to Richard Newman [:rnewman] from comment #21)
> > Dummy question:
> > 
> > browser/components/preferences/sync.js
> > browser/components/preferences/in-content/sync.js
> > 
> > Is one of these generated from the other? Why do we have both of these?
> 
> I... don't know!

Jared, can you shed some light on why this file exists as pretty much a total copy? Is there a path forward to *not* have it be a total copy? :D
Yes, there is a path forward. The in-content preferences are currently in development, and once enabled the windowed preferences implementation will be removed from the tree (hence removing the duplication).

I have a personal goal of getting the preferences enabled in Firefox 18.
Attachment #654891 - Flags: review?(rnewman) → review+
Comment on attachment 654897 [details] [diff] [review]
Part 8: Make SyncScheduler not a singleton, v1

Review of attachment 654897 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/policies.js
@@ +736,5 @@
>        Status.sync = PROLONGED_SYNC_FAILURE;
>        this._log.trace("shouldReportError: true (prolonged sync failure).");
>        return true;
>      }
>   

Trailing ws.

::: services/sync/tests/unit/test_interval_triggers.js
@@ +358,5 @@
>        server.stop(run_next_test);
>      }
>    };
>    
>    // Set nextSync != 0 

There's a lot of trailing whitespace in this file. Now's not a bad time to fix it, given the amount of blame-breakage.

::: services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
@@ +313,1 @@
>      

Likewise.
Attachment #654897 - Flags: review?(rnewman) → review+
Comment on attachment 655199 [details] [diff] [review]
Part 9: Begin work on engine de-singletonification, v2

Review of attachment 655199 [details] [diff] [review]:
-----------------------------------------------------------------

I think "r+" is the wrong flag to set for a patch that is known-broken, so f+ it is :D
Attachment #655199 - Flags: review?(rnewman) → feedback+
Comment on attachment 655201 [details] [diff] [review]
Part 10: Finish work from part 9; remove Clients singleton, v2

Review of attachment 655201 [details] [diff] [review]:
-----------------------------------------------------------------

This reads fine to me, but be aware that verifying its correctness in the context of the codebase as a whole is slightly beyond analytical review. If tests pass and Sync still works, I'll feel pretty confident. I trust your use of grep :D

Please feel free to append a Part 12 that just s/\s+$// on all of the files you've touched in this bug; we're really breaking blame here, so we might as well kill trailing whitespace as we go.

Please also fold this patch into the incomplete Part 9. Closer to a single working patch.
Attachment #655201 - Flags: review?(rnewman) → review+
Attachment #655147 - Flags: review?(rnewman) → review+
Blocks: 786489
Blocks: 787273
Blocks: 787306
Depends on: 815412
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.