Ran into a failure in our customization code, concerning the import order of modules and availability of configuration parameters – error reported being the equivalent of “no app context” ! Which then led me to this observation…
During the initialization of the app:
running in DEBUG mode, the app is initialized twice. Why?
the wrapper for application conf parameters, an instance of indico.core.config.IndicoConfig, is instantiated THREE times, and the 3 instances are used interchangedly when looking up param values. Why 3 instances? Are they actually interchangeable? Why not a singleton?
further, there is this strange pattern – the IndicoConfig class has an attribute _config that when not None is set to an werkzeug.datastructures.ImmutableDict instance with the conf param key/values. The FIRST time the IndicoConfig class is instantiated, self._config on it is ALWAYS None, the 2nd time it is instantiated self._config is ALWAYS an ImmutableDict, and the 3rd time it is instantiated its self._config is again ALWAYS None. (When self._config is None, the value lookup then falls on current_app.config[‘INDICO’] that, however, is not existing early on in the app loading time, and will fail with the error I mentioned above.) Is there a logic to this pattern?
I have tested the system making IndicoConfig wrapper class a singleton, and all seems to be well. Am going to paste the changed code sections below – also adding a number of print statements to highlight the points I am raising above.
Pasting here from my modified indico.core.config.py, from line 178 to end:
def load_config(only_defaults=False, override=None):
"""Load the configuration data.
:param only_defaults: Whether to load only the default options,
ignoring any user-specified config file
or environment-based overrides.
:param override: An optional dict with extra values to add to
the configuration. Any values provided here
will override values from the config file.
"""
data = dict(DEFAULTS, **INTERNAL_DEFAULTS)
if not only_defaults:
path = get_config_path()
config = _sanitize_data(_parse_config(path))
data.update(config)
env_override = os.environ.get('INDICO_CONF_OVERRIDE')
if env_override:
data.update(_sanitize_data(ast.literal_eval(env_override)))
resolved_path = resolve_link(path) if os.path.islink(path) else path
resolved_path = None if resolved_path == os.devnull else resolved_path
data['CONFIG_PATH'] = path
data['CONFIG_PATH_RESOLVED'] = resolved_path
print " >>>> LOADCONFIG ", only_defaults, override
print " >>>> LOADCONFIG INDICO", resolved_path
if resolved_path is not None:
data['LOGGING_CONFIG_PATH'] = os.path.join(os.path.dirname(resolved_path), data['LOGGING_CONFIG_FILE'])
print " >>>> LOADCONFIG LOG ", data['LOGGING_CONFIG_PATH']
if override:
data.update(_sanitize_data(override, allow_internal=True))
_postprocess_config(data)
return ImmutableDict(data)
class IndicoConfig(object):
"""Wrapper for the Indico configuration.
It exposes all config keys as read-only attributes.
Dynamic configuration attributes whose value may change depending
on other factors may be added as properties, but this should be
kept to a minimum and is mostly there for legacy reasons.
:param config: The dict containing the configuration data.
If omitted, it is taken from the active flask
application. An explicit configuration dict should
not be specified except in special cases such as
the initial app configuration where no app context
is available yet.
:param exc: The exception to raise when accessing an invalid
config key. This allows using the expected kind of
exception in most cases but overriding it when
exposing settings to Jinja where the default
:exc:`AttributeError` would silently be turned into
an empty string.
"""
__slots__ = ('_config', '_exc', '_count')
count = 0
_instance = None
def __new__(cls, *args, **kwargs):
cls.count += 1
print " >>>>>>>> NEW count={} instance={}".format(cls.count, cls._instance)
if not cls._instance:
cls._instance = super(IndicoConfig, cls).__new__(cls, *args, **kwargs)
return cls._instance
def __init__(self, config=None, exc=AttributeError):
# yuck, but we don't allow writing to attributes directly
object.__setattr__(self, '_config', config)
object.__setattr__(self, '_exc', exc)
object.__setattr__(self, '_count', self.__class__.count)
print " >>>>>>>> INIT count={} self={} self._config -> {}".format(self._count, self, type(self._config))
@property
def data(self):
try:
return self._config or current_app.config['INDICO']
except KeyError:
raise RuntimeError('config not loaded')
@property
def hash(self):
return crc32(repr(make_hashable(sorted(self.data.items()))))
@property
def CONFERENCE_CSS_TEMPLATES_BASE_URL(self):
return self.BASE_URL + '/css/confTemplates'
@property
def IMAGES_BASE_URL(self):
return 'static/images' if g.get('static_site') else url_parse('{}/images'.format(self.BASE_URL)).path
def __getattr__(self, name):
try:
print " >>>> LOOKUP count={} self={} name={} self._config -> {}".format(self._count, self, name, type(self._config))
return self.data[name]
except KeyError:
raise self._exc('no such setting: ' + name)
def __setattr__(self, key, value):
# and why is this not self._exc like elsewhere here?
raise AttributeError('cannot change config at runtime')
def __delattr__(self, key):
# and why is this not self._exc like elsewhere here?
raise AttributeError('cannot change config at runtime')
#: The global Indico configuration
config = IndicoConfig()
That’s because of the reloader of the dev server. It first loads the app and then restarts itself within the reloader process.
There is simply no benefit in using a singleton here. All the instance does is providing a wrapper that accesses the data passed to the constructor. It has absolutely no local state. So yes, all instances (that got passed the same data) are interchangeable.
And there is a reason for having separate instances: The one used in templates raises Exception instead of AttributeError, since the latter is converted to an Undefined object in Jinja, which renders as an empty string. But in non-template code we want the AttributeError since that’s the more appropriate exception class for it.
I think the main question here is: What are you doing with the config logic that would benefit from a single instance. Also keep in mind that the config is never available at import time.
When you are running custom code during “early app loading time” then you have two options: Run your code inside a with app.app_context(): ... block - that way you have the app context, and thus the global config will work. Or, you can do it like we did in configure_app and create an instance that explicitly uses app.config['INDICO'] as its data source (and thus doesn’t rely on the app context).
OK, good i asked then, because this reasoning is very well hidden in the code.
(The 3rd created instance of IndicoConfig is the one dedicated to Jinja.) Is it intentional that Jinja turns an AttributeError to an Undefined, but (presumably) it somehow renders an Exception ? If not, then it should be corrected in Jinja rather than the being carried cascaded into client code? And if yes, then would be very nice if the client code could maybe make this a little clearer?
And why is self._config None, then ImmutableDict, then None again?! Very disorienting if one is trying to unravel what the intention of it all is supposed to be.
In any case, in the situation this issue came up in i do not really need an app context – just need to pick off a specific conf param value. And calling make_app() etc to create another app instance with all that it requires causes is recursing the problem. Doing a small change that delays the particular import makes all work ok (except that it breaks the consistency of the coding pattern we had adopted in this particular case).
the global instance created in indico/core/config.py
the instance used inside configure_app
the instance used in Jinja
Tip: You can import traceback; traceback.print_stack() to print a traceback without an exception, which is very useful to see from where something gets called.
All well and good – but if I may repeat I do think that any and all strangeness, for whatever reason, of any given library should if at all possible be kept within the boundaries of that library; client code should not be made to suffer for it.
A further question: why _config is None in 1st and 3rd case, but ImmutableDict in the 2nd? And why these variations are not, at least, commented?
If someone fixes the issue linked above in Jinja I’ll be the first one to switch to StrictUndefined since it’s generally nicer to get hard errors for undefined values.
I thought I explained that in my last post It’s None when the config should be taken from the app context (see the data property). The only reason it’s set to a value in the second case is because that (temporary) instance is used without an app context and needs to have the data set explicitly.
Because they do not matter to someone using the class as intended, i.e. through the config object. Accessing the .data will return the correct data regardless of what _config is set to (as long as you are inside an app context), and so will accessing all the dynamic attributes for the actual config values.
It’s