ppkarwasz commented on code in PR #2517: URL: https://github.com/apache/logging-log4j2/pull/2517#discussion_r1580844634
########## log4j-api/src/main/java/org/apache/logging/log4j/spi/Provider.java: ########## Review Comment: On the contrary: you pointed out a valid concern. After looking at all `ThreadLocal` usages in Log4j, e.g. this one: https://github.com/apache/logging-log4j2/blob/cc876ae7c629b698b92414b7f3c94213cdd0a427/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java#L39 I came to the conclusion that our `log4j2.enableThreadlocals` contract can be: "if the value is `false` we **do** use `ThreadLocal`s, but we clear them up before returning to user code". The `ThreadContext` class is not different, but since the user has an almost direct access to the `ThreadLocal` the contract should be slightly different: "if you use `ThreadContext` properly and the value is `false` we **do** use `ThreadLocals`s, but you'll clear them up after your code executes". Therefore I believe that the algorithm should be: - if `ENABLE_THREADLOCALS` is `true`: use the garbage-free implementation (which does not clear the thread local after usage), - otherwise we use either copy-on-write or the default implementation. I think the former is more performant, but can cause classloader leaks if the user does not use it properly. -- 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]
