Skip to content

Commit

Permalink
add lock to cachetools usage
Browse files Browse the repository at this point in the history
* We observed daphne giving tracebacks when accessing logging settings.
  Originally, configure tower in tower settings was no a suspect because
  daphne is not multi-process. We've had issues with configure tower in
  tower settings and multi-process before. We later learned that Daphne
  is multi-threaded. Configure tower in tower was back to being a
  suspect. We constructed a minimal reproducer to show that multiple
  threads accessing settings can cause the same traceback that we saw in
  daphne. See
  https://gist.github.com/chrismeyersfsu/7aa4bdcf76e435efd617cb078c64d413
  for that recreator. These fixes stop the recreation.
  • Loading branch information
chrismeyersfsu authored and rebeccahhh committed Nov 19, 2021
1 parent 03ed6e9 commit afc9401
Showing 1 changed file with 24 additions and 1 deletion.
25 changes: 24 additions & 1 deletion awx/conf/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ def __init__(self, default_settings, cache, registry):
self.__dict__['_awx_conf_init_readonly'] = False
self.__dict__['cache'] = EncryptedCacheProxy(cache, registry)
self.__dict__['registry'] = registry
self.__dict__['_awx_conf_memoizedcache'] = cachetools.TTLCache(maxsize=2048, ttl=SETTING_MEMORY_TTL)
self.__dict__['_awx_conf_memoizedcache_lock'] = threading.Lock()

# record the current pid so we compare it post-fork for
# processes like the dispatcher and callback receiver
Expand Down Expand Up @@ -397,7 +399,11 @@ def _get_default(self, name):
def SETTINGS_MODULE(self):
return self._get_default('SETTINGS_MODULE')

@cachetools.cached(cache=cachetools.TTLCache(maxsize=2048, ttl=SETTING_MEMORY_TTL))
@cachetools.cachedmethod(
cache=lambda self: self.__dict__['_awx_conf_memoizedcache'],
key=lambda *args, **kwargs: SettingsWrapper.hashkey(*args, **kwargs),
lock=lambda self: self.__dict__['_awx_conf_memoizedcache_lock'],
)
def __getattr__(self, name):
value = empty
if name in self.all_supported_settings:
Expand Down Expand Up @@ -476,6 +482,23 @@ def is_overridden(self, setting):
set_on_default = getattr(self.default_settings, 'is_overridden', lambda s: False)(setting)
return set_locally or set_on_default

@classmethod
def hashkey(cls, *args, **kwargs):
"""
Usage of @cachetools.cached has changed to @cachetools.cachedmethod
The previous cachetools decorator called the hash function and passed in (self, key).
The new cachtools decorator calls the hash function with just (key).
Ideally, we would continue to pass self, however, the cachetools decorator interface
does not allow us to.
This hashkey function is to maintain that the key generated looks like
('<SettingsWrapper>', key). The thought is that maybe it is important to namespace
our cache to the SettingsWrapper scope in case some other usage of this cache exists.
I can not think of how any other system could and would use our private cache, but
for safety sake we are ensuring the key schema does not change.
"""
return cachetools.keys.hashkey(f"<{cls.__name__}>", *args, **kwargs)


def __getattr_without_cache__(self, name):
# Django 1.10 added an optimization to settings lookup:
Expand Down

0 comments on commit afc9401

Please sign in to comment.