-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport some Log4j API 3.x features #2392
Conversation
8882b25
to
2c9d831
Compare
As a curiosity, when working with GraalVM and either
|
2c9d831
to
09821aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really liked the new LoaderUtil#safeStream()
usage allowing GraalVM compat! 🤩
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/Tags.java
Outdated
Show resolved
Hide resolved
<bnd-extra-package-options> | ||
<!-- Not exported by most OSGi system bundles, hence we use the system classloader to load `sun.reflect.Reflection` --> | ||
!sun.reflect | ||
</bnd-extra-package-options> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this problem ever been reported? I feel like we are solving a problem of your C64 that you tinker at nights in your attic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A sun.reflect;resolution:=optional
declaration has been in log4j-api
since I joined the team.
We must have lost it, when we switched to BND, because it couldn't detect that we are using sun.reflect.Reflect
. However after I replaced LoaderUtil.loadClass
with Class.forName
the entry reappeared and broke our OSGi tests.
log4j-api/src/main/java/org/apache/logging/log4j/message/DefaultFlowMessageFactory.java
Outdated
Show resolved
Hide resolved
log4j-api/src/main/java/org/apache/logging/log4j/util/OsgiServiceLocator.java
Show resolved
Hide resolved
log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java
Outdated
Show resolved
Hide resolved
ed4c72e
to
3cfb7b5
Compare
We make `ServiceLoaderUtil` more GraalVM-friendly by replacing the usage of `MethodHandles.Lookup` with the requirement for the caller to instantiate the `ServiceLoader` himself.
By removing the reflective instantiation of `LoggerContextFactory` and `ThreadContextMap`, we make `log4j-to-jul` and `log4j-to-slf4j` more GraalVM-friendly.
5d989b6
to
a1c2bd1
Compare
*/ | ||
public class Resources { | ||
public final class Resources { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is used in @ResourceLock
JUnit 5 annotations.
Thanks to the DI
, most tests in Log4j Core 3.x will not require to access service classes through static methods (yes, even for ThreadContext
), but we want to mark those that need to be rewritten.
@vy, I am merging this PR in order to be able to use Please let me know if you find something else that needs to be corrected. |
We move some features found in the
main
branch oflog4j-api
back to2.x
:ServiceLoaderUtil#safeStream
method that takes an instance ofServiceLoader
as a parameter. Since now the caller must callServiceLoader#load
, we can remove the previous methods that created lambdas through reflection to workaround the caller-sensitivity ofServiceLoader#load
. Needless to say, the new approach is much more GraalVM-friendly,ReusableParameterizedMessage#set
methods public,@Deprecated
annotation fromSupplier
andBiConsumer
and let them extend thej.u.f
interfaces of the same name,MessageFactory
andFlowMessageFactory
,LoggerContextFactory
andThreadContextMap
for thelog4j-to-jul
andlog4j-to-slf4j
implementations.After these changes we reach two goals:
log4j-to-jul
andlog4j-to-slf4j
work with GraalVM without any additional reachability metadata, i.e. this provides a partial fix for Add GraalVM native compilation support #1539.