Return partially initialized ModelTypeStore synchronously |
||||||||
Issue descriptionWe've had integration test flakes for printers model type because they created a ModelTypeStore, and then tried to use it before it was initialized. We've gotten around this by running/flushing loops/pools and forcing the initialization to go through, but you can imagine a real use case where someone grabs your keyed service and tries to use it immediately, as a result of a user action, and try to write to it. This is going to fail as is. https://codereview.chromium.org/2799103002/ https://codereview.chromium.org/2758643002 It doesn't make sense to force any model type that has this or a similar pattern to implement some sort of queueing mechanism. It seems less than ideal to have these subtle race conditions peppering our code. It seems like the best solution is to implement some sort of queueing mechanism inside of ModelTypeStore. The main ramification of this is that if initialization fails, then these queued writes/reads are going to fail. Model types will need to be able to handle this, but since read/writes can already fail, the impact to model type code will likely be fairly minor.
,
Sep 19 2017
,
Sep 20 2017
"subtle" is a bit misleading. crbug.com/766661 reproduces this quite reliably.
,
Sep 20 2017
I'll make sure we talk about the prioritization of this bug today. However, in the short term, it may make more sense for a code change to the printers logic to stop assuming the ModelTypeStore is always ready.
,
Sep 20 2017
We're definitely not going to be able to get to this in for M63. We'd still fully intend to do this, but have no timeline at the moment.
,
Jan 17 2018
,
Feb 2 2018
I might pick this up soon.
,
Feb 2 2018
What I currently have in mind is a future-like loader object that replaces the current factory-callback: it would be similar to base::Optional and client code (e.g. bridges) would store it in a member field. In addition, it would expose a set of functions like PostWhenCreated.
,
Mar 6 2018
,
Mar 6 2018
CC+ Martin and Sylvain We hit this bug when trying to record the consent on iOS. The code reviewer pushed back as he believed the code is still flaky and we're just hiding the bug (I think he is correct) - CL where this happened https://chromium-review.googlesource.com/c/chromium/src/+/951484 My understanding from Martin is that this will be fixed in M67. I am thus marking this bug as assigned and raising its priority to P1. Markus: Feel free to reassign if you are not the right owner for this bug.
,
Mar 6 2018
I don't think the code in https://chromium-review.googlesource.com/c/chromium/src/+/951484 will be flaky in practice, as based on my understanding the time needed to initialize UserEventService is negligible, it just happens not to be synchronous, so we need to do it in advance. We're also adding a histogram to M66 to verify that claim. Sync team folks, please let me know if it is feasible to make initialization synchronous by M67. If not, we should at least add a callback so that the code has something to depend on.
,
Mar 6 2018
s/happens to be synchronous/happens to be *a*synchronous/
,
Jul 5
With all the mitigations in place (in-memory queuing), I don't think we expect further action here so closing the bug. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by s...@chromium.org
, Sep 1 2017