Skip to content
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

Register OptionHandlers through META-INF/services/annotations and Annotation Indexer rather than META-INF/services and Commons Discovery #9980

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

basil
Copy link
Member

@basil basil commented Nov 15, 2024

CLICommand has two mechanisms for registering OptionHandlers:

  1. /**
    * Auto-discovers {@link OptionHandler}s and add them to the given command line parser.
    */
    protected void registerOptionHandlers() {
    try {
    for (Class c : Index.list(OptionHandlerExtension.class, Jenkins.get().pluginManager.uberClassLoader, Class.class)) {
    Type t = Types.getBaseClass(c, OptionHandler.class);
    CmdLineParser.registerHandler(Types.erasure(Types.getTypeArgument(t, 0)), c);
    }
    } catch (IOException e) {
    throw new Error(e);
    }
    }
    which uses the Annotation Indexer to index (based on an @OptionHandlerExtension annotation) and load the handlers.
  2. static {
    // register option handlers that are defined
    ClassLoaders cls = new ClassLoaders();
    Jenkins j = Jenkins.getInstanceOrNull();
    if (j != null) { // only when running on the controller
    cls.put(j.getPluginManager().uberClassLoader);
    ResourceNameIterator servicesIter =
    new DiscoverServiceNames(cls).findResourceNames(OptionHandler.class.getName());
    final ResourceClassIterator itr =
    new DiscoverClasses(cls).findResourceClasses(servicesIter);
    while (itr.hasNext()) {
    Class h = itr.nextResourceClass().loadClass();
    Class c = Types.erasure(Types.getTypeArgument(Types.getBaseClass(h, OptionHandler.class), 0));
    CmdLineParser.registerHandler(c, h);
    }
    }
    }
    which uses a combination of Kohsuke's META-INF/services generator to index (based on a @MetaInfServices(OptionHandler.class) annotation) the handlers and Commons Discovery to load the handlers.

There is a desire to reduce and eventually remove usages of Commons Discovery, since it is an abandoned and unmaintained library. The second of these two code paths cannot be replaced with ServiceLoader, since ServiceLoader insists on having a zero-argument public constructor for each service, and OptionHandler violates this contract. So if getting rid of Commons Discovery is desired, then the second code path must be deprecated in favor of the first. Once all usages of the second code path in core and plugins have been converted to the first code path, and once those releases have been sufficiently deployed, then the second code path can be deleted.

This PR migrates from the second pattern to the first pattern in Jenkins core. I am aware of two cases where the second pattern is used in plugins:

I am not aware of any usages in proprietary plugins.

Implementation

The second code path was being invoked at class loading time (a single time) and loading the bulk of the handlers in practice there. The first code path was being invoked when each CLI command was invoked, which was simultaneously too frequent (a handler only needs to be registered once) and too late: HelpCommandTest started failing because we could not show the help until the handler had been loaded, and the handler was not loaded until the command was first invoked. We solved the latter problem by unifying the invocation of these two code paths from the same place: initialization of the class. In practice, this means that the existing handlers are loaded after this PR at the same time (and frequency) as they were before this PR, minimizing regressions and increasing consistency.

Testing done

mvn clean verify -Dtest=hudson.cli.AddJobToViewCommandTest,hudson.cli.BuildCommandTest,hudson.cli.CancelQuietDownCommandTest,hudson.cli.ClearQueueCommandTest,hudson.cli.ComputerStateTest,hudson.cli.ConnectNodeCommandTest,hudson.cli.ConsoleCommandTest,hudson.cli.CopyJobCommandTest,hudson.cli.CreateJobCommandTest,hudson.cli.CreateNodeCommandTest,hudson.cli.CreateViewCommandTest,hudson.cli.DeleteBuildsCommandTest,hudson.cli.DeleteJobCommandTest,hudson.cli.DeleteNodeCommandTest,hudson.cli.DeleteViewCommandTest,hudson.cli.DisablePluginCommandTest,hudson.cli.DisconnectNodeCommandTest,hudson.cli.EnableJobCommandTest,hudson.cli.EnablePluginCommandTest,hudson.cli.GetJobCommandTest,hudson.cli.GetNodeCommandTest,hudson.cli.GetViewCommandTest,hudson.cli.GroovyshCommandTest,hudson.cli.HelpCommandTest,hudson.cli.InstallPluginCommandTest,hudson.cli.ListJobsCommandTest,hudson.cli.ListPluginsCommandTest,hudson.cli.OfflineNodeCommandTest,hudson.cli.OnlineNodeCommandTest,hudson.cli.QuietDownCommandTest,hudson.cli.ReloadConfigurationCommandTest,hudson.cli.ReloadJobCommandTest,hudson.cli.RemoveJobFromViewCommandTest,hudson.cli.RunRangeCommand2Test,hudson.cli.RunRangeCommandTest,hudson.cli.SetBuildDescriptionCommandTest,hudson.cli.SetBuildDisplayNameCommandTest,hudson.cli.UpdateNodeCommandTest,hudson.cli.UpdateViewCommandTest,hudson.cli.ViewManipulationTestBase,hudson.cli.WaitNodeOfflineCommandTest,hudson.cli.WaitNodeOnlineCommandTest,hudson.model.ComputerSetTest,hudson.model.ItemsTest,hudson.model.listeners.ItemListenerTest,hudson.util.RobustReflectionConverterTest,jenkins.cli.StopBuildsCommandTest,jenkins.model.JenkinsManagePermissionTest,jenkins.model.NodeListenerTest,jenkins.model.RunIdMigratorTest,jenkins.model.ScriptListenerTest,jenkins.security.Security3314Test,jenkins.security.Security3448Test,lib.form.PasswordTest

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

… Annotation Indexer rather than `META-INF/services` and Commons Discovery
@basil basil added the skip-changelog Should not be shown in the changelog label Nov 15, 2024
Comment on lines +569 to +572
for (Class c : Index.list(OptionHandlerExtension.class, Jenkins.get().pluginManager.uberClassLoader, Class.class)) {
Type t = Types.getBaseClass(c, OptionHandler.class);
CmdLineParser.registerHandler(Types.erasure(Types.getTypeArgument(t, 0)), c);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from lines 535-537, so please do not review this as original code authored by me.

@@ -35,7 +35,6 @@

/**
* {@link OptionHandler}s that should be auto-discovered.
* TODO is this actually necessary? {@code @MetaInfServices(OptionHandler.class)} seems to work as well.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answer: it is actually necessary if we want to remove Commons Discovery someday, as ServiceLoader cannot work with the OptionHandler classes that have no public zero-argument constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants