> 2) Races, start-up races everywhere This seems easy to fix by having the observer return a promise, and wrapping it with .then().
> 3) It hides lock management from the caller I personally feel that getting rid of the library is going to create more races. I think the helper library is a nice abstraction, and if done correctly can help us reduce race conditions. > I personally did a implementation in Keyboard app too [1], but it might not be general enough. What were the reasons to re-invent the settings wrapper instead of making it better? Can you post an example of a settings observer with and without the library? Currently my preference is to keep the existing library and make it better. I do not think that we should keep inventing additional wrappers in each app, and should instead take some time and make the version in shared/ better. Best, Kevin On Sat, Jul 25, 2015 at 8:39 AM, Greg Weng <gw...@mozilla.com> wrote: > To be clear, I'm not looking to a perfect solution that can replace > the listener wonderfully. What I said is if you or others have a > solution already laid on there, why we don't just pick it and use it > to replace the problematic listener? > > And I also think It's unrealistic to ban people to invent any wrappers > if the API is indeed not convenient enough or just with too much > details that could be simplified in different contexts (and I believe > you need to send mails like this again and again to hack all the new > wrappers if you really dislike the approach). > > Anyway, back to the case, I think it's good to stop using it. I just > hope people could discuss the practices we'd already invented here, so > we could know what we better to know while rewriting the code, instead > of fail into the same traps that you and others and me (yes, I have a > specific wrapper made from the past mistakes, too) had suffered from. > > 2015-07-25 23:24 GMT+08:00 Tim Guan-tin Chien <timdr...@mozilla.com>: > > Thanks for you comment! > > > > Those are good points, but I specifically don't want to do things like > > I-have-a-prefect-library-developed-in-secret-everyone-should-use-it. > > This e-mail is more like a lesson learned retro and ask people to > > interact with the API directly or invent their own wheels with these > > experiences in mind. At least stop using SettingsListener.observe() > > without at least aware it's problems. > > > > There are a few points worth mentioning from my impl though: > > > > 1. Setting values can be read through one async method and one sync > method. > > 2. The sync method will throw if the value is not there. > > 3. The async method will return a promise that would return to an > > object that keep the values, not the values themselves. This is done > > so that the same promise could be returned and kept by the callers, > > but the values can be updated by the observer callback. > > > > I generally prefer an organic process from which we find the most > > useful solution from marketplace of ideas. As long as ideas considered > > our past mistakes, I don't really care if Gaia as a whole have higher > > entropy or not. > > > > > > On Sat, Jul 25, 2015 at 6:18 PM, Greg Weng <snowma...@gmail.com> wrote: > >> I support to deprecate it because of the issues you mentioned. However, > when > >> people tried to wrap an API again and again, it obviously something > failed > >> to make people happy to use the API without any wrappers, although some > >> debts are from the time we don't have any better way to manage async > flow > >> like Promise promised. > >> > >> So, IMO we may better to find an usable (hopefully compact and nice to > use) > >> alternative before we wipe out the code use the listener.Maybe Tim could > >> elaborate how to use his solution to solve the common issues in Gaia, > with a > >> gist or readme under the app, and we could replace the listener with > that > >> stepwise. Or, if it's not general enough, we could evaluate different > >> solutions with clear descriptions. At least, we should have some clear > >> practices about how to replace it with the semantically equal snippets > >> manually manipulating the API, if we don't expect an one-man army to > submit > >> a 3K+ lines patch across different components to do that. Without such > best > >> practices, people involve in may submit and land different patches with > very > >> different way from callback to Promise to handle the similar issues in > >> different components, which would definitely add the entropy of our > >> codebase, not eradicate that. > >> > >> 2015-07-25 17:32 GMT+08:00 Tim Guan-tin Chien <timdr...@mozilla.com>: > >>> > >>> Hi, > >>> > >>> SettingsListener was one of the oldest piece of Gaia, and it even > >>> reaches into Gecko b2g/. However for years it's problem has not been > >>> widely communicated. > >>> > >>> == Problems == > >>> > >>> 1) The "default value" on the 2nd parameter > >>> > >>> It asks for a "default value" on the 2nd parameter, and it will use it > >>> when there is no mozSettings available and when mozSettings returns > >>> undefined. Both behavior does not make sense nowadays because: > >>> > >>> 1.1) It makes no sense to return a fail safe value when mozSettings > >>> breaks. We also don't expect Gaia apps to run on environments w/o > >>> mozSettings. > >>> 1.2) It creates two or more places in the code base in which people > >>> would need update the "default value". The only place the default > >>> value should live is the common-settings.json, which is the preload > >>> values of mozSettings. We should use it and bump the db version in > >>> Gecko to make the new value follow into the profile, instead of > >>> duplicate that value in the code base. > >>> > >>> 2) Races, start-up races everywhere > >>> > >>> The callback will be invoked when the value is first returned (via a > >>> get() call), and when the installed observer invokes due to the change > >>> in value. Before the first value returns, the value remain unknown in > >>> the JS world but users of SettingsListener.observe() usually assume > >>> user will not reach their code before getting that value. That's a > >>> false assumption. The code ended up behaves incorrectly with an > >>> undefined value (or, with an default value which assumed safe but may > >>> not be). > >>> > >>> 3) It hides lock management from the caller > >>> > >>> The current implementation hides the lock inside the library, and > >>> reuse if in the same function loop, but in fact lock exists to > >>> facilitate ... lock on the database changes. There will be some > >>> follow-up on the problem with mozSettings itself but in the mean time > >>> we should stop using an helper library that hide locks and introduce > >>> footguns around possible setting change races. > >>> > >>> == Proposal == > >>> > >>> To my surprises, people have not moving away from it already on other > >>> apps. Either all the uses have carefully workaround the problems > >>> stated, or the bugs are still there. > >>> > >>> My proposal here is simply deprecate SettingsListener and have each of > >>> the users interact with mozSettings directly, or have each of the apps > >>> develop libraries they feel comfortable. > >>> > >>> Async flow in JS is way better than what we have at the beginning of > >>> the project, thank to mostly the introduction of Promise. We have also > >>> learned a lot through bugs and regressions. Given the fact DOMRequest > >>> is already a then-able, it's shouldn't be hard to manage the code and > >>> make it only do things (or response to things) only until the setting > >>> value is available. > >>> > >>> Even in just one System app, there are already multiple library-level > >>> implementations that attempt to wrap mozSettings. Without saying which > >>> one is the best one, since the problem with SettingsListener should be > >>> understood, I would assume these implementations should not be > >>> suffering the problem that SettingsListener have. > >>> > >>> I personally did a implementation in Keyboard app too [1], but it > >>> might not be general enough. > >>> > >>> If this is a generally agreed path to move forward, we should start > >>> doing it and new patches should stop using SettingsListener. > >>> > >>> We would have to deal with the same thing in Gecko too. I don't know > >>> who will be responsible of that though. > >>> > >>> Thoughts? > >>> > >>> > >>> Tim > >>> > >>> PS A related topic would be a thread named "mozSettings API refinement > >>> proposal" started by Alive (hat tip!). Proposal was generally > >>> perceived well but left unimplemented beyond DOMRequest#then. We > >>> should get better at tracking these things. > >>> > >>> [1] > >>> > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/settings.js > >>> _______________________________________________ > >>> dev-gaia mailing list > >>> dev-g...@lists.mozilla.org > >>> https://lists.mozilla.org/listinfo/dev-gaia > >> > >> > >> > >> > >> -- > >> Greg Weng > >> > >> http://about.me/snowmantw > >> > >> Understand y f = f [ y f ] ; lose last remaining non-major friend > >> -- Anonymous > > _______________________________________________ > > dev-gaia mailing list > > dev-g...@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-gaia > _______________________________________________ > dev-gaia mailing list > dev-g...@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-gaia >
_______________________________________________ dev-b2g mailing list dev-b2g@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-b2g
