GNOME Bugzilla – Bug 629331
BackendStore.load_backends and the prepare() functions should be idempotent.
Last modified: 2010-09-22 16:14:11 UTC
The documentation doesn't say whether they are or not. They clearly should be, since it could be dangerous to perform their work multiple times.
Created attachment 170001 [details] [review] Make BackendStore.load_backends() and the prepare() functions idempotent Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo629331-idempotency
Review of attachment 170001 [details] [review]: ::: folks/backend-store.vala @@ +134,3 @@ */ public async void load_backends () throws GLib.Error { + if (!this.backends_loaded) This isn't threadsafe either. ::: folks/individual-aggregator.vala @@ +156,3 @@ + if (this.is_prepared) + return; + This isn't threadsafe. If we're going to care about being threadsafe (which we probably should), the whole function should be protected by a lock on this.is_prepared. ::: folks/persona-store.vala @@ +160,3 @@ + * @since 0.1.17 + */ + public bool is_prepared { get; protected set; default = false; } When going through the C API of libfolks, I found that Vala will actually generate a public folks_persona_store_set_is_prepared() function if you use "protected set" (since subclasses which aren't in-tree need to be able to set the property). The best solution I could come up with was to have the following in the superclass: public abstract bool is_prepared { get; } and the following in the subclass: public override bool is_prepared { get { return this._is_prepared; } } That does mean that each subclass has to copy and paste the boilerplate property override code and declarations, though, and also call this.notify_property ("is-prepared") at the correct times. @@ +200,3 @@ * If this function throws an error, the PersonaStore will not be functional. + * + * This function is guaranteed to be idempotent (since version 0.1.17). 0.1.18…or 0.2.0.
Also, the definition of BackendStore.backends_loaded needs to be moved into this branch from http://git.collabora.co.uk/?p=user/treitter/folks.git;a=commitdiff;h=511d01030b271688384533a4a6c99849b24a26b3.
(Mass changing milestones; please search for this phrase to delete the bug spam.)
Due to bug #629081, BackendStore.load_backends() shouldn't be made idempotent by simply bailing out if it's been run before: it needs to scan for backends again (as backends could've been re-enabled since it was last called), but only load backends that it finds which haven't been loaded before.
(In reply to comment #5) > Due to bug #629081, BackendStore.load_backends() shouldn't be made idempotent > by simply bailing out if it's been run before: it needs to scan for backends > again (as backends could've been re-enabled since it was last called), but only > load backends that it finds which haven't been loaded before. I'm going to target this branch for merging first, so we'll fix that issue in that branch.
Created attachment 170795 [details] [review] Make BackendStore.load_backends() and the prepare() functions idempotent Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo629331-idempotency4
Review of attachment 170795 [details] [review]: I think the approach for making BackendStore.load_backends() idempotent needs to be changed. An entry in the NEWS file should also be added (major changes, API changes and bugs fixed). ::: backends/key-file/kf-backend.vala @@ +37,3 @@ + * Whether this Backend has been prepared. + * + * See {@link Folks.Backend.is_prepared}. Probably need a @since line here. ::: backends/key-file/kf-persona-store.vala @@ +57,3 @@ + * Whether this PersonaStore has been prepared. + * + * See {@link Folks.PersonaStore.is_prepared}. Probably need a @since line here. ::: backends/telepathy/lib/tpf-persona-store.vala @@ +96,3 @@ + * Whether this PersonaStore has been prepared. + * + * See {@link Folks.PersonaStore.is_prepared}. Probably need a @since line here. ::: backends/telepathy/tp-backend.vala @@ +57,3 @@ + * Whether this Backend has been prepared. + * + * See {@link Folks.Backend.is_prepared}. Probably need a @since line here. ::: folks/backend-store.vala @@ +134,2 @@ { + if (!this._backends_loaded) As I mentioned on IRC, I think it would be best if we didn't make BackendStore.load_backends() idempotent in that it is only run once. I think we should make it idempotent by having it run the code to search for modules every time it's executed, but only actually loading modules which it hasn't loaded before. This would allow the following code to work: backend_store.load_backends (); // Load all the backends except "foo", which is disabled backend_store.enable_backend ("foo"); backend_store.load_backends (); // Load only the "foo" backend
(In reply to comment #8) > Review of attachment 170795 [details] [review]: Added the missing @since lines > ::: folks/backend-store.vala > @@ +134,2 @@ > { > + if (!this._backends_loaded) > > As I mentioned on IRC, I think it would be best if we didn't make > BackendStore.load_backends() idempotent in that it is only run once. I think we > should make it idempotent by having it run the code to search for modules every > time it's executed, but only actually loading modules which it hasn't loaded > before. This would allow the following code to work: > backend_store.load_backends (); // Load all the backends except "foo", which > is disabled > backend_store.enable_backend ("foo"); > backend_store.load_backends (); // Load only the "foo" backend As discussed on IRC, I'll fix this in the branch for bug #629081 (since it needs more work anyhow), so I can merge outstanding branches sooner.
commit 42612cdf772bc6a03d2f43bbbc3568d1439227c2 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Sep 9 09:24:11 2010 -0700 Add Backend.is_prepared and make prepare() idempotent. Fixes bgo#629331. NEWS | 3 ++ backends/key-file/kf-backend.vala | 66 +++++++++++++++++++++++++----------- backends/telepathy/tp-backend.vala | 47 +++++++++++++++++++------ folks/backend.vala | 10 +++++ 4 files changed, 95 insertions(+), 31 deletions(-) commit ae8cd14af5ffe741830019355bcf2c66943c8759 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Sep 9 09:10:22 2010 -0700 Add IndividualAggregator.is_prepared and make prepare() idempotent. Helps bgo#629331. folks/individual-aggregator.vala | 25 ++++++++++++++++++++++++- 1 files changed, 24 insertions(+), 1 deletions(-) commit 329c41f971e0aad006b880c2640a30d38b459675 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Sep 9 09:07:11 2010 -0700 Add PersonaStore.is_prepared and make prepare() idempotent. Helps bgo#629331. backends/key-file/kf-persona-store.vala | 186 ++++++++++++++----------- backends/telepathy/lib/tpf-persona-store.vala | 131 ++++++++++------- folks/persona-store.vala | 10 ++ 3 files changed, 194 insertions(+), 133 deletions(-) commit 5c035298f3a0fe33f0b53798d66d4f3e73014a25 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Sep 8 16:15:02 2010 -0700 Make BackendStore.load_backends() idempotent. Helps bgo#629331. folks/backend-store.vala | 92 ++++++++++++++++++++++++++------------------- 1 files changed, 53 insertions(+), 39 deletions(-)