Closed Bug 763563 Opened 12 years ago Closed 7 years ago

mozSettings.getLock().get() should be synchronous

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: vingtetun, Unassigned)

Details

While there are reasons for mozSettings.getLock().set() to be asynchronous, it's unclear why mozSettings.get() can't be a sync API with cached values.
Summary: mozSettings..getLock().get() should be synchronous → mozSettings.getLock().get() should be synchronous
The problem is that you can easily get race conditions between multiple processes if you rely on cached values. Maybe we don't care about that for settings, but I bet at some point somebody's going to use the Settings API as a persistent cross-app message passing system... just like it's being done with prefs in Gecko already.
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> I bet at some point somebody's going to use the Settings API
> as a persistent cross-app message passing system

Over my dead body! ;)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> (In reply to Philipp von Weitershausen [:philikon] from comment #1)
> > I bet at some point somebody's going to use the Settings API
> > as a persistent cross-app message passing system
> 
> Over my dead body! ;)

Well, nothing prevents web apps to do that...
(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #1)
> > > I bet at some point somebody's going to use the Settings API
> > > as a persistent cross-app message passing system
> > 
> > Over my dead body! ;)
> 
> Well, nothing prevents web apps to do that...

My understanding is that we want an initial list of settings and we don't allow adding new entries into the DB during runtime.
We don't enforce that now but it should be an easy fix. (Once we have a list of initial settings)
What Gregor said.

We have to specify the semantics of all the entries in the settings DB or else the API is pretty much meaningless, and we should have just used cross-domain IDB.
Oh I wasn't aware of that! That definitely reduces the risk of creating scenarios where you'd run into races.
Note, I'm not commenting on whether this API should be synchronous.  I actually think getLock() should be async, but I'm afraid that ship may have sailed already.  AIUI in the current implementation, content can block arbitrarily long waiting to acquire the "settings lock".  That's bad.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Note, I'm not commenting on whether this API should be synchronous.  I
> actually think getLock() should be async, but I'm afraid that ship may have
> sailed already.  AIUI in the current implementation, content can block
> arbitrarily long waiting to acquire the "settings lock".  That's bad.

Ha we had this discussion before. getLock is async. We just create an internal lock object and queue the requests on it. on the next tick we process it and do all the DB work.
So internally, acquiring the lock is async?  Or do we block on acquiring the lock in the "next tick"?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> So internally, acquiring the lock is async?  Or do we block on acquiring the
> lock in the "next tick"?

The internal lock object is a regular JS object with a queue for requests and a boolean lock field. The lock closes automatically on the next tick.

So the getLock function creates an internal lock object. We register a callback function that processes all requests for the lock on the next tick and return the lock object or the get/set calls.

Once the callback function is called, we request a transaction object from IDB and process all the DB requests for a lock. So we move all the blocking part to IDB.
OK, I see.  You use the main thread's event queue to serialize request processing.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Note, I'm not commenting on whether this API should be synchronous.  I
> actually think getLock() should be async, but I'm afraid that ship may have
> sailed already.  AIUI in the current implementation, content can block
> arbitrarily long waiting to acquire the "settings lock".  That's bad.

Having getLock() async makes perfect sense, but why do we need a lock for reading a value? Can't all the dirty logic be hidden under the hood of the API and then there could be a mozSettings.get instead of a mozSettings.getLock().get()?
Getting a value will require blocking file IO, in general.  If gaia apps go unresponsive for a few seconds while reading DB values, I'll strangle someone ;).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Getting a value will require blocking file IO, in general.  If gaia apps go
> unresponsive for a few seconds while reading DB values, I'll strangle
> someone ;).

Sure! But if values are cached, this means it will happen only for the first read. All other will be fine.
And I would still strangle someone! ;)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> And I would still strangle someone! ;)

The reason why I have raised this issue is because (don't strangle me!) it's tempting to store the value in localStoragein order to get synchronicity when you really want it.
localStorage forces synchronous main-thread disk IO.  r- on any gaia patches that use it.

Sorry, but user experience trumps small dev overhead of async programming style.  At least you guys get to use a language with lexical closures ... ;)
(In reply to Vivien Nicolas (:vingtetun) from comment #14)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > Getting a value will require blocking file IO, in general.  If gaia apps go
> > unresponsive for a few seconds while reading DB values, I'll strangle
> > someone ;).
> 
> Sure! But if values are cached, this means it will happen only for the first
> read. All other will be fine.

Can't we cache the values at startup?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> localStorage forces synchronous main-thread disk IO.  r- on any gaia patches
> that use it.
> 
> Sorry, but user experience trumps small dev overhead of async programming
> style.  At least you guys get to use a language with lexical closures ... ;)

You can't r- web apps developer. My feeling is that if Vivien is tempted to use localStorage to prevent the async pain, other less talented developers might not think twice about it.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) from comment #14)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > > Getting a value will require blocking file IO, in general.  If gaia apps go
> > > unresponsive for a few seconds while reading DB values, I'll strangle
> > > someone ;).
> > 
> > Sure! But if values are cached, this means it will happen only for the first
> > read. All other will be fine.
> 
> Can't we cache the values at startup?
> 

That would lock the entire DB into memory.  We don't want to do that unless we're absolutely confident we can bound it to a small size, O(100KB) or so.

> (In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> > localStorage forces synchronous main-thread disk IO.  r- on any gaia patches
> > that use it.
> > 
> > Sorry, but user experience trumps small dev overhead of async programming
> > style.  At least you guys get to use a language with lexical closures ... ;)
> 
> You can't r- web apps developer. My feeling is that if Vivien is tempted to
> use localStorage to prevent the async pain, other less talented developers
> might not think twice about it.

Sure, and they'll write apps that get reviewed badly.  Let's prevent jank in review now instead of b2g dying on the free market from bad UX.
Also, gaia should set the best example possible to web developers.  "Less talented web developers do this, so why not use it in gaia" is a bad argument.
My point wasn't that we should make Gaia's UX bad to reduce our developers burden but that we should probably seriously consider solving that issue if even Gaia developers are suffering from it and are tempted to work around it.
Usually, when writing specifications, we tend to consider that if anything very awful can be done, it will more than likely happen in the Web.

In the other hand, if there is clearly no way to do that (like the DB being too big to be stored in memory), we should definitely keep the async design.
(In reply to Vivien Nicolas (:vingtetun) from comment #16)
> The reason why I have raised this issue is because (don't strangle me!) it's
> tempting to store the value in localStoragein order to get synchronicity
> when you really want it.

Can we disable localStorage in gaia apps? I'm 90% serious.
(In reply to ben turner [:bent] from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) from comment #16)
> > The reason why I have raised this issue is because (don't strangle me!) it's
> > tempting to store the value in localStoragein order to get synchronicity
> > when you really want it.
> 
> Can we disable localStorage in gaia apps? I'm 90% serious.

+0.95
Vivien, can you give an example where you need this? I don't like the idea of copying the whole DB to every window and try to keep it in sync with the real DB.
Do you want it for all settings or just a subset of them?
You also get notified when a setting changes and you can listen now on individual setting-changes as well.
(In reply to Gregor Wagner [:gwagner] from comment #24)
> Vivien, can you give an example where you need this? I don't like the idea
> of copying the whole DB to every window and try to keep it in sync with the
> real DB.

When Gaia starts we want to check if the lockscreen is enabled or not before showing/hiding it. This is a simple boolean value and beeing async here result in a bad UX because we need a solution which is:
 - show the lockscreen first and then hide it if it is disabled.
 - do not show the lockscreen first and then show if it is enabled.
 - do not show any content until a value is returned.

There is not that much example I can give except this one thought.
(In reply to ben turner [:bent] from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) from comment #16)
> > The reason why I have raised this issue is because (don't strangle me!) it's
> > tempting to store the value in localStoragein order to get synchronicity
> > when you really want it.
> 
> Can we disable localStorage in gaia apps? I'm 90% serious.

+ 100%

Best to keep LS from gaining any more foothold.
(In reply to Vivien Nicolas (:vingtetun) from comment #25)
> (In reply to Gregor Wagner [:gwagner] from comment #24)
> > Vivien, can you give an example where you need this? I don't like the idea
> > of copying the whole DB to every window and try to keep it in sync with the
> > real DB.
> 
> When Gaia starts we want to check if the lockscreen is enabled or not before
> showing/hiding it. This is a simple boolean value and beeing async here
> result in a bad UX because we need a solution which is:
>  - show the lockscreen first and then hide it if it is disabled.
>  - do not show the lockscreen first and then show if it is enabled.
>  - do not show any content until a value is returned.
> 
> There is not that much example I can give except this one thought.

Is it taking so long to read the value? If this value and only that one was cached on startup so you will get it quickly (after one pass trough the event loop), would that be okay regarding that specific use case?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #26)
> (In reply to ben turner [:bent] from comment #22)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #16)
> > > The reason why I have raised this issue is because (don't strangle me!) it's
> > > tempting to store the value in localStoragein order to get synchronicity
> > > when you really want it.
> > 
> > Can we disable localStorage in gaia apps? I'm 90% serious.
> 
> + 100%
> 
> Best to keep LS from gaining any more foothold.

That would be good
FxOS/Gonk has been removed from the codebase. Mass-invalidating FxOS related Device Interface bugs to clean up the component. 

If I incorrectly invalidated something, please let me know.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Bulk correction of resolution of B2G bugs to INCOMPLETE.
Resolution: INVALID → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.