Fixed Java LDAP tests in JDK 17 #2430
Conversation
github-actions
bot
added
agent-java
community
|
👋 @tobiasstadler Thanks a lot for your contribution! It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it. Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it. |
| <plugin> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <argLine>@{argLine} --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED</argLine> |
There was a problem hiding this comment.
What happens if someone uses Java 17 to run their app and does add this export? Will the agent break their app or throw errors?
There was a problem hiding this comment.
As exceptions, that are thrown from plugins, are caught/handled by ByteBuddy/the agent, I assume the plugin won't work, but the rest of the app work.
There was a problem hiding this comment.
That's true but these exceptions still affect the application - more overhead due to creation and logging and the logs are going to be spammed, too.
Just disabling the module on Java 17 is probably also not the best option as even Java 11 can be configured to enforce the module boundaries instead of warning about them.
I think we'll probably have to remove the LDAP plugin until we have found a solution for that.
There was a problem hiding this comment.
Ok, I will create an external plugin in the mean time.
There was a problem hiding this comment.
Are you taking care of the removal or should I?
There was a problem hiding this comment.
classLoaderCanLoadClass("com.sun.jndi.ldap.Connection") seems to work.
There was a problem hiding this comment.
Seems to work in the sense that it returns false on Java 17 without --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED but true with that flag?
There was a problem hiding this comment.
Hm it doesn't work with --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED.
But
public class LdapClientAdvice {
static {
try {
com.sun.jndi.ldap.LdapResult.class.getConstructor().newInstance();
} catch (Exception e) {
throw new IllegalStateException(e);
}
}
...
works with --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED and "only" logs one exception without --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED
There was a problem hiding this comment.
The downside of that is that this eagerly links com.sun.jndi.ldap.LdapResult and initializes a bunch of classes that otherwise would not be initialized at this point. We usually try not to impact the order in which classes are loaded as this can negatively impact the application. For example, if java.util.logging is used somewhere in the prematurely initialized classes, app server can't set up their custom log handlers.
There was a problem hiding this comment.
yes, this is true.
❕ Build Aborted
Expand to view the summary
Build stats
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
2 participants
What does this PR do?
Fixed test on JDK 17
Checklist