1980348 - Remove thread marshalling from stores

Status: RESOLVED FIXED

(Firefox for Android :: Tooling, task)

Description

[Polly [:polly]]

Our current implementation of Store spins up a new thread each time state is accessed. This makes the call site's code needlessly complex and error-prone as we often have to pull together composite state from multiple asynchronous requests.

If we remove the thread creation and handling from the Store, this would allow users of a Store to make their own decisions about which thread to use. Typically, we expect main thread to be used as the Store contains accessors for in-memory objects - we don't expect these to be long-running operations.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox for Android::Tooling' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Polly [:polly]]

Attachment: Bug 1980348 - remove thread marshalling from Stores.

This required a few changes:

1. the Store production code in lib-state no longer spawns a new thread when you dispatch a store action.
2. Because of this, a lot of tests need to have .join() or .joinBlocking() removed from where they dispatch actions.
3. Most tests pass after this. But a few fail because either:

• exceptions were being thrown on background store threads
• they relied on store actions not having completed before assertions

Comment 3

[Polly [:polly]]

Attachment: Bug 1980348 - fix test that was giving non-deterministic results.

@Ignore-d this in the previous patch, so i'm fixing that separately.
This test manipulates Tab objects, which are being transitioned between active and inactive states by a time-dependent Flow.
We're testing the storage of these objects, so i've pulled out the logic to compare tabs to its own assertTabSame() function, which looks at the contents of the tabs rather than whether they're currently active.

Also don't think we need to use spy() here for the class under test, so i've dropped in the real thing.

Comment 4

[Polly [:polly]]

Attachment: Bug 1980348 - get rid of Store.waitUntilIdle() :yay-frog:

We don't need this any more with single threaded stores, so we can clean up the tests quite a bit.

Comment 5

[Polly [:polly]]

Attachment: Bug 1980348 - remove thread marshalling from Stores.

Second attempt with a smaller patch size. (see patch https://phabricator.services.mozilla.com/D263733 for first attempt)
For now Store.dispatch() returns a dummy Job.
This means the api, which is used in a lot of tests, doesn't need to be changed as part of this patch.
This is temporary and will be cleaned up as soon as we are happy with this change - intention is to have the dispatch function not return and clean up all the join()/joinBlocking() thread wrangling in unit tests.

Changes that have been made:

• remove threading code from Store
• fix tests than rely on multithreading or throw exceptions on background threads

Comment 6

[Polly [:polly]]

Attachment: Bug 1980348 - change Store.dispatch() api to no longer return a Job.

This means we have to delete join() and joinBlocking() everywhere they are appended to Store.dispatch() (mostly in the unit tests)

Comment 7

[Polly [:polly]]

Attachment: Bug 1980348 - get rid of Store.waitUntilIdle().

This was previously just a passthrough method so we can remove it now. This simplifies a lot of tests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3bc61ed0f6bb
https://hg.mozilla.org/integration/autoland/rev/eda8bc9983d6
remove thread marshalling from Stores. r=android-reviewers,matt-tighe

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/eda8bc9983d6

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b35ac0e38259
https://hg.mozilla.org/integration/autoland/rev/ee96a94e9c0a
change Store.dispatch() api to no longer return a Job. r=android-reviewers,007
https://github.com/mozilla-firefox/firefox/commit/e3a00b853568
https://hg.mozilla.org/integration/autoland/rev/343ae3f24eb5
get rid of Store.waitUntilIdle(). r=android-reviewers,007

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/ee96a94e9c0a
https://hg.mozilla.org/mozilla-central/rev/343ae3f24eb5