Wrap the autofill implementation in an Arc<> to help the sync manager. #3935

mhammond · 2021-03-17T05:40:23Z

This patch works-around the lack of the functionality described
in mozilla/uniffi-rs#417.

In short, it splits the store into 2 parts - the
uniffi-exposed "wrapper" and the actual implementation.
This "wrapper" wraps the implementation in an Arc<Mutex>>, so
that it can hand a clone of this Arc<> to the sync manager, so
that it can construct sync engines, causing us to end up with
multiple references to the Store being alive but without any
being actual & references.

(Thinking some more about it, I probably could have done this
slightly differently, and just wrapped the DB itself, but I
ended up somewhat automatically taking this route because this
is what the shape would naturally be if uniffi helped us.
I think passing the entire store around does make more sense
in the longer term anyway (eg, there might be other state beyond
the DB) and is what the other components do, but it does make
this patch bigger than it would otherwise be)

It also makes some changes in the sync manager, really just
enough to convince me that it will work - there will be plenty
of other places that will need similar treatment. I chose to
use the SyncEngine trait exclusively there, which I think makes
sense because ideally we'd use that trait everywhere to avoid the
Sync Manager knowing about the implementation (but it would still
require it to link to all those crates, so that's just a
code-simplicity issue)

mhammond · 2021-03-17T05:43:51Z

The other thing I meant to say is that as we discussed, this inverts how the sync manager gets engines. Currently, something (ie, Fenix code) needs a reference to both places and the sync manager and calls (something like) manager.set_places(places).

However, in this world, Fenix only needs a reference to the autofill store, and can do just autofill.offer_to_sync_manager() - and magically, a sync manager that's created in the future (or even one already created) will pick it up. I'm obviously open to a better name.

mhammond · 2021-03-17T05:46:23Z

cc @jhugman for context on that uniffi ticket.

lougeniaC64 · 2021-03-22T20:10:27Z

components/autofill/src/db/store.rs

+// This is the type that uniffi exposes. It holds an `Arc<Mutex<>>` around the
+// actual implementation, because we need access to the `Arc<Mutex<>>` for
+// integration with the sync manager. Our UDL also defines us as being
+// "ThreadSafe" to we manage our own locking via the `Mutex` - but this means


nit: "so" we manage?

lougeniaC64 · 2021-03-22T20:45:35Z

components/autofill/src/db/store.rs

+// in practice we are actually held via an `Arc<Arc<Mutex<>>>` - but one day
+// https://github.com/mozilla/uniffi-rs/issues/417 will give us access to the
+// `Arc<>` uniffi owns, which means we can drop this mess entirely (ie, `Store`)
+// can then just have a mutex around the DB.
 pub struct Store {


Does the Store struct need to be public? It looks like StoreImpl is the type that is being imported. Or if both should be public maybe an explanation as to which should be used in what circumstances. There may be an easy explanation that I'm missing, but I could see myself getting confused in the future.

This got confusing quickly and sadly they both need to be pub. Store needs to be pub because the sync manager uses it (StoreImpl has the Arc<Store> which we clone and hand to the sync manager, so it gets the Store) and Store needs to be pub because autofill-utils needs it to get the sync engines.

So yeah, this sucks and I added some comments, but hopefully we can get rid of this confusion with the proposed autofill enhancements.

mhammond · 2021-03-23T01:11:42Z

I'm about to put up a new version that doesn't wrap the entire store in an Arc<Mutex> - instead the store is just wrapped in an Arc<> and the DB remains wrapped in the Mutex it already has. This is both a smaller patch and what's technically needed currently. Later in the future we might need to shuffle this around a bit, but I think this is better.

codecov-io · 2021-03-23T03:26:19Z

Codecov Report

Merging #3935 (6f533f2) into main (43c0835) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main   #3935     +/-   ##
=======================================
  Coverage   0.00%   0.00%             
=======================================
  Files        101      75     -26     
  Lines       5903    4439   -1464     
=======================================
+ Misses      5903    4439   -1464

Impacted Files	Coverage Δ
components/autofill/src/db/mod.rs	`0.00% <0.00%> (ø)`
components/support/guid/src/lib.rs	`0.00% <0.00%> (ø)`
components/support/types/src/lib.rs	`0.00% <0.00%> (ø)`
components/support/jwcrypto/src/lib.rs	`0.00% <0.00%> (ø)`
components/support/sql/src/conn_ext.rs	`0.00% <0.00%> (ø)`
components/support/sql/src/interrupt.rs	`0.00% <0.00%> (ø)`
components/support/sync15-traits/src/telemetry.rs	`0.00% <0.00%> (ø)`
components/autofill/src/sync/mod.rs
components/autofill/src/db/models/mod.rs
...mponents/autofill/src/sync/credit_card/incoming.rs
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43c0835...6f533f2. Read the comment docs.

lougeniaC64

LGTM!

This patch works-around the lack of the functionlity described in mozilla/uniffi-rs#417. In short, it splits the store into 2 parts - the uniffi-exposed "wrapper" and the actual implementation. This "wrapper" wraps the implementation in an `Arc<>`, so that it can hand a clone of this `Arc<>` to the sync manager, so that it can construct sync engines, causing us to end up with multiple references to the `Store` being alive but without any being actual `&` references. The database itself continues to be wrapped in a `Mutex<>` It also makes some changes in the sync manager, really just enough to convince me that it will work - there will be plenty of other places that will need similar treatment. I chose to use the `SyncEngine` trait exclusively there, which I think makes sense because ideally we'd use that trait *everywhere* to avoid the Sync Manager knowing about the implementation (but it would still require it to link to all those crates, so that's just a code-simplicity issue)

mhammond requested review from rfk and lougeniaC64 March 17, 2021 05:40

lougeniaC64 reviewed Mar 22, 2021

View reviewed changes

mhammond force-pushed the arc-workaround branch from 4e6569b to 705477c Compare March 23, 2021 01:13

mhammond marked this pull request as ready for review March 23, 2021 01:13

mhammond requested a review from lougeniaC64 March 23, 2021 01:13

mhammond force-pushed the arc-workaround branch from 705477c to 6f533f2 Compare March 23, 2021 03:14

mhammond changed the title ~~Wrap the autofill implementation in an Arc<Mutex<>> to help the sync manager.~~ Wrap the autofill implementation in an Arc<> to help the sync manager. Mar 23, 2021

lougeniaC64 approved these changes Mar 24, 2021

View reviewed changes

mhammond force-pushed the arc-workaround branch from 6f533f2 to c38ce09 Compare March 24, 2021 23:56

mhammond merged commit 8784c84 into mozilla:main Mar 25, 2021

mhammond deleted the arc-workaround branch March 25, 2021 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap the autofill implementation in an Arc<> to help the sync manager. #3935

Wrap the autofill implementation in an Arc<> to help the sync manager. #3935

mhammond commented Mar 17, 2021

mhammond commented Mar 17, 2021

mhammond commented Mar 17, 2021

lougeniaC64 Mar 22, 2021

lougeniaC64 Mar 22, 2021 •

edited

Loading

mhammond Mar 23, 2021

mhammond commented Mar 23, 2021

codecov-io commented Mar 23, 2021

lougeniaC64 left a comment

Wrap the autofill implementation in an Arc<> to help the sync manager. #3935

Wrap the autofill implementation in an Arc<> to help the sync manager. #3935

Conversation

mhammond commented Mar 17, 2021

mhammond commented Mar 17, 2021

mhammond commented Mar 17, 2021

lougeniaC64 Mar 22, 2021

Choose a reason for hiding this comment

lougeniaC64 Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

mhammond Mar 23, 2021

Choose a reason for hiding this comment

mhammond commented Mar 23, 2021

codecov-io commented Mar 23, 2021

Codecov Report

lougeniaC64 left a comment

Choose a reason for hiding this comment

lougeniaC64 Mar 22, 2021 •

edited

Loading