rgoers commented on PR #1401: URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1550731405
> But things have changed, in a positive way: we now have a powerful DI system. Let's put that into use! Unfortunately, the DI system as it is today is broken. I discovered this when working on context properties and Matt is aware of this. The problem is that we have a bunch of "Constants" that really just check a property when the "constant" is initialized. However, that makes it impossible to make those LoggerContext specific. In addition, if you don't happen to have the "correct" Injector instance you are kind of screwed. I'm not saying these can't be fixed, but the DI system is not going to solve every problem in Log4j. Certainly not the items I am raising here. > Yes and with any other system run by a decent DI, you shouldn't need static access. JTL contains the most complicated DI usage out there and there is not a single call to static DI.* methods. Yes, the DI system doesn't have a static anchor. Instead we have many DI instances anchored in many places. Our DI system does have static methods (which I don't have a problem with). Again, the DI system really has nothing to do with what I am asking for. > I have provided several examples demonstrating why static registries are bad: > > Hinders testing No they don't. So long as you can register mock objects it makes no difference whether the registry has a static anchor or not. > Hinders composition (e.g., one cannot provide a PropertiesUtil implementation, etc.) Huh? That would be a horrible mistake. That is like saying you cannot provide a DI implementation. Somethings shouldn't be replaceable. > It is a bad-practice in a DI-backed system If our DI system worked properly I might agree with you. It does not. Note - a "Registry" does not necessarily require to be anchored by a static. It could very well be anchored using the DI system, if that indeed was possible (it currently wouldn't be). > Scoped Values are indeed on the horizon and we should cater for that. We might indeed need to massage the current Recycler API to accommodate that. In particular, scoped values require a lambda (i.e., ScopedValue.where(var, val).run(Runnable)), whereas our current recycler requires explicit acquire/release calls. These ergonomics are pretty much orthongonal. I will try to flesh out some Recycler API support for this. Here you are pretty much repeating back to me what I said. > I have my reservations about this Ralph. Recycler is merely an efficient object pool. I think requesting thread-related functionality would break that assumption and sophisticate the design. Do you have a particular use case in mind that requires, e.g., thread inheritance? Well of course. I have two. 1. Scoped Values. "Scoped values are automatically inherited by all child threads created using the StructuredTaskScope." Given that is the default I am sure we will want a way to disable it for certain use cases. 2. I am using an InheritableThreadLocal in PropretiesUtil so that when configuration takes place any method called, even on a child thread, will get the correct property sources. > Do you have a [non-hypothetical] use case in mind for the recycler sharing you mention? Well, maybe. I simply haven't looked. But we use so many ThreadLocals that I have a suspicion that a few of them share the same lifecycle. In that case, in my mind it makes sense to share the recycler. But again, without inspecting every use of ThreadLocal in Log4j, no I can't firmly give you an example. > Let me again share a similar example from Spring. Imagine a Tree bean implementing Lifecycle. I have to stop you right there. The problem in Log4j is that we have several Lifecycles: 1. The entire JVM 2. The application instance (there can be several in a web container or OSGi). Generally, this will correlate to a LoggerContext. 3. The configuration. As you know these can start and stop many times during the lifetime of a LoggerContext. 4. AppenderManagers - These may live as long as the LoggerContext does or as short as each Configuration they are part of, depending on how the configuration changed. 5. Even a LogEvent has a lifecycle of sorts. Matt made a good attempt to make the DI system deal with all these different lifecycles in an appropriate manner, except it doesn't quite work (which probably isn't the fault of the DI code itself but other stuff that he didn't fix - like constants that shouldn't be constants. I will not that you also didn't address the idea of using a Spec object instead of a simple String that directly names the implementation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
