Redo the avatar backend state storage to fix saving and sync problems.
Review Request #9484 — Created Jan. 15, 2018 and submitted
Custom avatar backends provided by extensions have had some problems
staying enabled or staying as defaults, for a handful of reasons. Some
of these were fixed in Djblets 1.0.1 by preventing the avatar registry
from aggressively disabling backends it couldn't find, but that only
solved one half of the problem.The other half is that the registry kept its own local state on the
enabled/default avatar backends, which wasn't always correct and wasn't
ever kept in sync with the configuration state changes from other web
servers or processes. Avatar state could load before extensions loaded,
and if the default avatar backend was from the extension, that instance
of the registry would end up seeing it as None. Future threads might get
the right value, however. Even if the configuration was fixed to re-set
the default backend, any existing registries would continue to have the
stale state. On top of this, setting a list of enabled backends would
result in premature saves, which would hit the database more than
necessary and could result in other state issues.The main fix for this is to remove the local state tracking, and always
use the currentSiteConfiguration
instead. All methods that look up
state will fetch the current configuration and read from that, ensuring
that if the configuration had recently reloaded, the new state will be
used.All methods that save configuration will write it to the configuration
as well. This has a couple side effects. First, if some code wrote
configuration without saving (save=False
), and then waiting a while to
save, it's possible that the written configuration could be lost (no
different than any other work withSiteConfiguration
). Second, if
configuration was written without saving, and the current
SiteConfiguration
was then saved manually, the new avatar
configuration would be saved, without ever calling
AvatarServiceRegistry.save()
. In practice, this should all be fine,
and is necessary to deal with the state synchronization issues.Along with this, the
enabled_services
property has been replaced with
aset_enabled_services()
function, so that asave=False
can be
passed. That function also no longer does more than one save.Unit tests have been updated to check the local and in-database settings
for all the mutable operations.
All unit tests for Djblets and Review Board pass.
Played around with the avatar configuration locally. Didn't hit any
immediate issues. Settings seemed to have persisted correctly and
triggeredSiteConfiguration
reloads.
Description | From | Last Updated |
---|---|---|
F841 local variable 'siteconfig_defaults' is assigned to but never used |
reviewbot |