Manipulating ETSConfig in Application. _initialize_application_home is problematic

[corranwebster]

In Application._initialize_application_home, the application_home directory of ETSConfig is mutated:

envisage/envisage/application.py

Lines 486 to 495 in 6e5c7ba

	def _initialize_application_home(self):
	""" Initialize the application home directory. """
	ETSConfig.application_home = os.path.join(
	ETSConfig.application_data, self.id
	)
	# Make sure it exists!
	if not os.path.exists(ETSConfig.application_home):
	os.makedirs(ETSConfig.application_home)

This is good in the sense that it brings the two into alignment if they are different, and it's definitely good that the method makes sure that the directory exists(!), but that allows the possibility of objects created prior to the application having the old value. The most likely case is for Preferences objects to get the wrong thing, in code that creates a custom preferences object, like the (not unreasonable) following:

from importlib.resources import as_file, files

with as_file(files(app.mod) / "default.ini) as path:
     preferences = ScopedPreferences()
     preferences.get_scope("default").load(path)

# later
app = Application(preferences=preferences)

The implementation of ScopedPreferences.get_scope as a side-effect sets the default value of its application_preferences_filename trait using the value of ETSConfig.application_home before the creation of the Application object changes this, so it will potentially target the wrong file (in a directory that may not even exist).

Even worse than the above example, it is possible that even the default preferences could end up being created before _initialize_application_home is called if, for example, traits listeners are set up on it in the class in a way that causes early instantiation of the preferences from the default (eg. something that listens for changes to the preferences instance to hook up and disconnect preference listeners).

To some extent, this boils down to "do not mess with global state that you do not own."

I'm not sure what the fix should be.

[mdickinson]

To some extent, this boils down to "do not mess with global state that you do not own."

Agreed, but it would make sense that the application does own its choices about directories. The problem is that information which really belongs to the app is being stored in a different place that's accessible to and used by things that are unaware of the existence of the application context. It seems to me that this information belongs in the app and not in ETSConfig at all.

Could we deprecate use of ETSConfig.application_home entirely and move the home directory choices to the Application class here? Who else is using application_home, and for what purposes?

[mdickinson]

I guess making the application the single source of truth for directory information would require restructuring so that any Preferences class is aware of the application it belongs to. That makes sense to me conceptually, but it may be hard to make that change in a reasonably backwards compatible way.

[corranwebster]

This just bit me again in a different way: assigning a different value to home (eg. during testing, to force it to write to some other location; or to follow OS conventions on app directory names) doesn't work.

There is also a race condition during application start-up between the default value for the home trait and this being called: _initialize_application_home is called after traits are initialized, so anything which causes the default value for home to be used during trait initialization (eg. trait observers, which may be chained and so not immediately obvious that they are using home) will cause home to be populated with a different value than ETSConfig.application_home.

[corranwebster]

any Preferences class is aware of the application it belongs to

No, it just needs to be aware of the right directory to use. And currently the application is creating the Preferences object, so it can tell it the right place to look. So I think this can be done without breakage except in cases where developers are creating their own Preferences. So I think that's safe.

[corranwebster]

Could we deprecate use of ETSConfig.application_home entirely and move the home directory choices to the Application class here? Who else is using application_home, and for what purposes?

I think that's the right thing to do for the Application class (and its subclasses), other than having home default to ETSConfig.application_home if one isn't supplied.