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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-72111] Add Lifecycle#set #8555

Closed
wants to merge 1 commit into from
Closed

[JENKINS-72111] Add Lifecycle#set #8555

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions core/src/main/java/hudson/lifecycle/Lifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,22 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.ExtensionPoint;
import hudson.Functions;
import hudson.PluginManager;
import hudson.Util;
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.reflect.InvocationTargetException;
import java.nio.file.Files;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import jenkins.util.SystemProperties;
import org.apache.commons.io.FileUtils;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.Beta;

/**
* Provides the capability for starting/stopping/restarting/uninstalling Hudson.
Expand All @@ -57,9 +61,8 @@

/**
* Gets the singleton instance.
*
* @return never null
*/
@NonNull
public static synchronized Lifecycle get() {
if (INSTANCE == null) {
Lifecycle instance;
Expand Down Expand Up @@ -136,6 +139,19 @@
return INSTANCE;
}

/**
* Sets the singleton instance.
* Appropriate for implementations defined in plugins,
* since {@link #get} may be called before plugins are initialized,
* and so it is not safe to pass a plugin-defined class to the system property {@code hudson.lifecycle}
* despite the use of {@link PluginManager#uberClassLoader}.
Comment on lines +144 to +147
Copy link
Member

@jtnord jtnord Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this comment is contradictory to me.

if this method is appropriate for plugins, but itself may be called before plugins are initialized, how can a plugin ensure that they can call set before get as any consumers of get can rightly assume that the lifecycle is fixed.
the answer is they can not - so it is not really "safe" to be called in a plugin.

once get is called - to all purposes INSTANCE should be final and never change.
You could probably demonstrate this by throwing an exception if INSTANCE is non null (which means get has been called)

so whilst setting the property is not safe to use with an implementation from a plugin equally this new method appears to not be safe as callers of get can get intermediate responses.

Copy link
Member

Choose a reason for hiding this comment

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

how can a plugin ensure that they can call set before get

It cannot, but it can call set in an @Initializer or otherwise during startup.

any consumers of get can rightly assume that the lifecycle is fixed

Why would a consumer make that assumption? I did a search of @jenkinsci sources and found no example of a consumer doing anything with the return value other than immediately calling some method on it.

Copy link
Member

Choose a reason for hiding this comment

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

It cannot, but it can call set in an @Initializer or otherwise during startup.

which means if startup fails the lifecycle will not be correctly replaced leading to "the wrong" lifecyle handling the issue?

Why would a consumer make that assumption

Because it was a single initialized only once in code.

I did a search of https://github.com/jenkinsci sources and found no example of a consumer doing anything with the return value other than immediately calling some method on it.

an example (but a bad one) is

jenkins/core/src/main/java/hudson/lifecycle/WindowsInstallerLink.java

Lines 267 to 288 in 7d2b484

public static WindowsInstallerLink registerIfApplicable() {
if (!Functions.isWindows())
return null; // this is a Windows only feature
if (Lifecycle.get() instanceof WindowsServiceLifecycle)
return null; // already installed as Windows service
// this system property is set by the launcher when we run "java -jar jenkins.war"
// and this is how we know where is jenkins.war.
String war = SystemProperties.getString("executable-war");
if (war != null && new File(war).exists()) {
WindowsInstallerLink link = new WindowsInstallerLink(new File(war));
// TODO possibly now unused (JNLP installation mode is long gone):
if (SystemProperties.getString(WindowsInstallerLink.class.getName() + ".prominent") != null)
Jenkins.get().getActions().add(link);
return link;
}
return null;
}
that changes behaviour once and once only for example at startup as it has assumed that the Lifecycle will not change.

I hope to rip that example out in #8523 so this becomes a moot point.

I think this just needs some clearer documentation that this is super advanced and you should not normally do this - and it you do that you will not be called back during early initialisation like a regular Lifecycle and that even changing it could cause other plugin code that is obtaining the lifecycle (say to check if you can restart once) may not observe any changes in the lifecycle. (or documewnt in the get that the lifecycle can change at runtime)
esp the class level comment is now incorrect about how the Lifecycle is found.

Copy link
Member

Choose a reason for hiding this comment

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

documentation that this is super advanced and you should not normally do this

Well, it is marked Beta, but yes documentation should be adjusted.

There are probably other ways to approach the problem. Currently

jenkins/core/src/main/java/hudson/model/RestartListener.java

Lines 24 to 27 in 7d2b484

/**
* Called immediately before the restart is actually triggered.
*/
public void onRestart() {}
offers no “veto” functionality, and so there is no other way to intercept

jenkins/core/src/main/java/jenkins/model/Jenkins.java

Lines 4649 to 4663 in 7d2b484

public void restart() throws RestartNotSupportedException {
final Lifecycle lifecycle = restartableLifecycle();
servletContext.setAttribute("app", new HudsonIsRestarting());
new Thread("restart thread") {
final String exitUser = getAuthentication2().getName();
@Override
public void run() {
try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {
// give some time for the browser to load the "reloading" page
lifecycle.onStatusUpdate("Restart in 5 seconds");
Thread.sleep(TimeUnit.SECONDS.toMillis(5));
lifecycle.onStop(exitUser, null);
Listeners.notify(RestartListener.class, true, RestartListener::onRestart);
lifecycle.restart();
or

jenkins/core/src/main/java/jenkins/model/Jenkins.java

Lines 4689 to 4710 in 7d2b484

public void safeRestart(String message) throws RestartNotSupportedException {
final Lifecycle lifecycle = restartableLifecycle();
// Quiet down so that we won't launch new builds.
quietDownInfo = new QuietDownInfo(message, true);
new Thread("safe-restart thread") {
final String exitUser = getAuthentication2().getName();
@Override
public void run() {
try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {
// Wait 'til we have no active executors.
doQuietDown(true, 0, message, true);
// Make sure isQuietingDown is still true.
if (isQuietingDown()) {
servletContext.setAttribute("app", new HudsonIsRestarting(true));
// give some time for the browser to load the "reloading" page
lifecycle.onStatusUpdate("Restart in 10 seconds");
Thread.sleep(TimeUnit.SECONDS.toMillis(10));
lifecycle.onStop(exitUser, null);
Listeners.notify(RestartListener.class, true, RestartListener::onRestart);
lifecycle.restart();
Alternately, it is possible (I suppose) to define a Lifecycle in a module in the same class loader as jenkins-core.jar, though this is logistically more complicated because then you need to package this module somewhere, and it cannot easily call other plugin methods, etc. The current patch is more expedient.

Copy link
Member

Choose a reason for hiding this comment

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

(for the avoidance of doubt my comments are non blocking)

Comment on lines +145 to +147
Copy link
Member

Choose a reason for hiding this comment

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

While a veto-type method in RestartListener (as in #8555 (comment)) would probably be best, it occurs to me that we do not really need a set method. It would suffice for

jenkins/core/src/main/java/hudson/lifecycle/Lifecycle.java

Lines 83 to 86 in d1b8473

} catch (ClassNotFoundException e) {
NoClassDefFoundError x = new NoClassDefFoundError(e.getMessage());
x.initCause(e);
throw x;
to quietly fall back to the default, but then have an initializer run after plugins are loaded which tries again (and complains noisily if the class still cannot be found).

I do not think #8555 (comment) is a real issue; it just means that for example a plugin-provided lifecycle cannot override onStop for the corner case of a boot failure. (The CloudBees CI implementation only needs to override restart, which should never be called before startup is complete so far as I know.)

* @since TODO
*/
@Restricted(Beta.class)
public static synchronized void set(@NonNull Lifecycle lifecycle) {
INSTANCE = Objects.requireNonNull(lifecycle);

Check warning on line 152 in core/src/main/java/hudson/lifecycle/Lifecycle.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 152 is not covered by tests
}

Check warning on line 153 in core/src/main/java/hudson/lifecycle/Lifecycle.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 153 is not covered by tests

/**
* If the location of {@code jenkins.war} is known in this life cycle,
* return it location. Otherwise return null to indicate that it is unknown.
Expand Down
Loading