Deprecating Extension.optional

[jglick]

Plugin developers often grab for this flag because it is easy to find, but it is almost always the wrong choice. It means the extension loader will still try to load the extension; it will just be less noisy (still printing a warning to the log, one line rather than a stack trace!) if it fails. And it is actually tricky to get it to fail. You have to arrange for the initialization of the extension class (and perhaps also the constructor, I do not recall) to fail. So if your plugin has an optional dep on a plugin xxx-api and you write

@Extension(optional = true)
public class MyImpl implements XXXExtensionPoint {
    @Override
    public XXXType someXXXMethod() {
        // …
    }
}

then that is fine, because types from xxx-api are required in the signature of MyImpl, so the NoClassDefFoundError will be caught by the extension loader and this extension will be skipped. But people often try to do something like the following:

@Extension(optional = true)
public class MyListener extends RunListener {
    @Override
    public void onStarted(Run build) {
        if (build instanceof XXXBuild xxxBuild) {
            // …
        }
    }
}

This will be quietly registered even when xxx-api is disabled—and then throw NoClassDefFoundErrors every time it is used! That is because the HotSpot linker is lazy: even though some method bodies refer to an inaccessible type, they are not linked until the first time the method is run.

You can use optional = true correctly if you think carefully about what you are doing, and write tests with RealJenkinsRule.omitPlugins to prove that it behaves as expected. But you might as well just use @OptionalExtension which is much easier to reason about (and avoids the one-line warning to boot).

Testing done

None.

Proposed changelog entries

• Deprecating the Extension.optional attribute.

Proposed changelog category

/label developer

Proposed upgrade guidelines

N/A

Maintainer checklist

• There are at least two (2) approvals for the pull request and no outstanding requests for change.
• Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
• Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
• Proper changelog labels are set so that the changelog can be generated automatically.
• If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[jglick]

Unclear why this was “optional”. Of course this would only make sense on servers with systemd. But it would get loaded as an extension in all cases.

[jglick]

And Lifecycle does not even use the extension loader to begin with! Traditionally ExtensionPoint was just used as a very hand-wavy indicator for “things you might want to pay attention to in Javadoc as a plugin author”, rather than specifically meaning “a type which can be subtyped and marked with @Extension to register”. (A broad category of such mistakes is placing ExtensionPoint on a Describable. It is the Descriptor which is the extension point.)

[daniel-beck]

Traditionally ExtensionPoint was just used as a very hand-wavy indicator for “things you might want to pay attention to in Javadoc as a plugin author”, rather than specifically meaning “a type which can be subtyped and marked with @Extension to register”.

Isn't that an indicator this should stay?

See also class Javadoc for PluginServletFilter in which this quirk is made explicit.

[jglick]

jenkins/core/src/main/java/hudson/util/PluginServletFilter.java

Lines 57 to 58 in c9eaa5d

	* While this class by itself is not an extension point, I'm marking this class
	* as an extension point so that this class will be more discoverable.

predates #7892 which is a true extension point.

[daniel-beck]

The point stands, it's not just tradition, but also documented as such.

[jglick]

Then we can correct the documentation: #11210

[daniel-beck]

If the idea is that ExtensionPoint is only for things intended to be @Extension'ed, then

jenkins/core/src/main/java/hudson/ExtensionPoint.java

Lines 35 to 41 in c97fecc

	* Marker interface that designates extensible components
	* in Jenkins that can be implemented by plugins.
	*
	* <p>
	* See respective interfaces/classes for more about how to register custom
	* implementations to Jenkins. See {@link Extension} for how to have
	* Jenkins auto-discover your implementations.

should also be rephrased accordingly. At the moment it's ambiguous (at least to me), d4c6a4b kept the original documentation referring to the specific classes explaining how to register (which would apply to the case of PluginServletFilter), only adding a mention of @Extension.

[jglick]

Who knows.

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[A1exKH]

LGTM.