PR created: https://github.com/apache/logging-log4j2/pull/1136
Would appreciate reviews over the next couple days. — Matt Sicker > On Nov 7, 2022, at 12:29, Matt Sicker <m...@musigma.org> wrote: > > I’ve got a much simpler refactor for API initialization which avoids > introducing new SPIs (i.e., it re-uses the Provider service as-is). I’ll file > it as a PR later today. It can likely be improved further (mostly related to > the PropertyEnvironment init and injection), but it’s an improvement at least > for centralizing most of the startup logic into a single class (basically a > rewrite of ProviderUtil). One major change I was able to make in this, > though, was simplifying the OSGi bundle activators quite a bit. The API > activator mainly needs to acquire an initialization lock and set up a bundle > event listener to wait for other bundles to be installed. Once a bundle is > installed that actually has a Log4j Provider to load from it, it can register > its providers via ServiceRegistry (rather than directly invoking > ProviderUtil) and then unlock the initialization lock. Using > ServiceLoader::load in the bundle change listener seems to work well enough > (only disadvantage to this approach is that it requires giving the Log4j > bundles some high level permissions in order to acquire different bundles’ > ClassLoader or BundleWiring objects, but that assumes the user is using both > OSGi and a SecurityManager at the same time which just sounds dreadful), and > it can also remove a bit of custom OSGi service registration or lookup logic. > > If we go as far as providing a full lifecycle for, say, LoggerContextFactory > (and not just the LoggerContext objects which already have a lifecycle), then > it could potentially be shutdown from the bundle activator as well, though > that might already rely on the ShutdownCallbackRegistry. > >> On Nov 7, 2022, at 12:19 AM, Ralph Goers <ralph.go...@dslextreme.com> wrote: >> >> Note that PropertiesUtil will need some work in master to implement >> https://cwiki.apache.org/confluence/display/LOGGING/Properties+Enhancement >> <https://cwiki.apache.org/confluence/display/LOGGING/Properties+Enhancement>. >> >> Ralph >> >>> On Nov 6, 2022, at 6:37 PM, Matt Sicker <m...@musigma.org> wrote: >>> >>> On the LowLevelLogUtil front, I’ve updated that to use StatusLogger now >>> once StatusLogger has initialized. The code could probably be backported to >>> 2.x. >>> >>> As for how PropertiesUtil caches values, simplifications there would be >>> great! >>> — >>> Matt Sicker >>> >>>> On Nov 6, 2022, at 03:37, Piotr P. Karwasz <piotr.karw...@gmail.com> wrote: >>>> >>>> Hi Matt, >>>> >>>> On Sat, 5 Nov 2022 at 22:32, Matt Sicker <m...@musigma.org> wrote: >>>>> After an attempt at soloing the idea, I’ve reverted my changes and am >>>>> instead documenting the current system along with adding some proposals >>>>> in the following Confluence doc [0]. I’ve analyzed the initialization >>>>> ordering of the API (without going into detail of how log4j-core >>>>> initializes as this differs fairly significantly in 2.x and 3.x at this >>>>> point). Right now, some of the main differences between 2.x and master is >>>>> that most of the documented static fields from initialization are >>>>> currently refactored into Lazy<T> static fields, though that only defers >>>>> initialization until slightly later. >>>> >>>> Thanks for analyzing and writing down this quite complex initialization >>>> process. >>>> >>>> My main concerns with the way this works now are: >>>> >>>> 1. Logging of errors in the pre-StatusLogger phase is inconsistent. >>>> Some classes use `LowLevelLogUtil`, some ignore errors, while the >>>> usage of StatusLogger I introduced in `ServiceLoaderUtil` is brittle: >>>> it requires to call `loadServices` with `verbose = false` to disable >>>> the usage of `StatusLogger`. I think that the ideal solution would be >>>> to remove enough static initialization from `StatusLogger` and >>>> `AbstractLogger` (i.e. move it to constructors) so that >>>> LowLevelLogUtil can create a `StatusLogger` with a hardcoded >>>> configuration. Later in the initialization process we can replace it >>>> with the real `StatusLogger`. >>>> >>>> 2. There are still a couple of singleton classes that cache the result >>>> of `PropertiesUtil.getProperty` in private static fields. I think >>>> these calls can be easily moved to the constructor of the class. E.g. >>>> `StatusLogger` could easily have a constructor that does not require >>>> `PropertiesUtil` at all. >>>> >>>> 3. The way `PropertiesUtil` caches values could be simplified (cf. >>>> discussion in https://issues.apache.org/jira/browse/LOG4J2-3621): it >>>> caches all values from environment variables and Java system >>>> properties at startup. Most of the time none of those values configure >>>> Log4j2. On the other hand missing values are not cached, so most of >>>> the time a lookup for `log4j2.configurationFile` will find no value in >>>> the cache, perform a lookup in all available property sources and >>>> return `null`. >>>> >>>> Most of these changes can also be done in `release-2.x` without >>>> breaking public and protected members. >>>> >>>> Piotr >>> >> >
