Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-38487] - Jenkins startup must not fail in the case of …
…ComputerListener failure (#2610)

Without this code Jenkinbs startup fails if EnvInject fails to find global property file on startup.

Javadoc says "Exceptions will be recorded to the listener. Note that throwing an exception doesn't put the computer offline." regarding the listener method exception, hence we should not block Jenkins startup
(cherry picked from commit 58e1228)
  • Loading branch information
oleg-nenashev authored and olivergondza committed Nov 8, 2016
1 parent 7e8ea4a commit b104407
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 6 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/slaves/ComputerListener.java
Expand Up @@ -143,7 +143,7 @@ public void onOnline(Computer c) {}
* Starting Hudson 1.312, this method is also invoked for the master, not just for agents.
*
* @param listener
* This is connected to the launch log of the computer.
* This is connected to the launch log of the computer or Jenkins master.
* Since this method is called synchronously from the thread
* that launches a computer, if this method performs a time-consuming
* operation, this listener should be notified of the progress.
Expand Down
24 changes: 19 additions & 5 deletions core/src/main/java/jenkins/model/Jenkins.java
Expand Up @@ -957,11 +957,25 @@ protected void doRun() throws Exception {

updateComputerList();

{// master is online now
Computer c = toComputer();
if(c!=null)
for (ComputerListener cl : ComputerListener.all())
cl.onOnline(c, new LogTaskListener(LOGGER, INFO));
{// master is online now, it's instance must always exist
final Computer c = toComputer();
if(c != null) {
for (ComputerListener cl : ComputerListener.all()) {
try {
cl.onOnline(c, new LogTaskListener(LOGGER, INFO));
} catch (Throwable t) {
if (t instanceof Error) {
// We propagate Runtime errors, because they are fatal.
throw t;
}

// Other exceptions should be logged instead of failing the Jenkins startup (See listener's Javadoc)
// We also throw it for InterruptedException since it's what is expected according to the javadoc
LOGGER.log(SEVERE, String.format("Invocation of the computer listener %s failed for the Jenkins master node",
cl.getClass()), t);
}
}
}
}

for (ItemListener l : ItemListener.all()) {
Expand Down
19 changes: 19 additions & 0 deletions test/src/test/java/jenkins/model/JenkinsTest.java
Expand Up @@ -42,6 +42,7 @@

import hudson.maven.MavenModuleSet;
import hudson.maven.MavenModuleSetBuild;
import hudson.model.Computer;
import hudson.model.Failure;
import hudson.model.RestartListener;
import hudson.model.RootAction;
Expand All @@ -51,6 +52,7 @@
import hudson.security.HudsonPrivateSecurityRealm;
import hudson.util.HttpResponses;
import hudson.model.FreeStyleProject;
import hudson.model.TaskListener;
import hudson.security.GlobalMatrixAuthorizationStrategy;
import hudson.security.LegacySecurityRealm;
import hudson.security.Permission;
Expand All @@ -70,6 +72,7 @@
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;

import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.URL;

Expand Down Expand Up @@ -452,4 +455,20 @@ public void runScriptOnOfflineComputer() throws Exception {
assertThat(rsp.getContentAsString(), containsString("Node is offline"));
assertThat(rsp.getStatusCode(), equalTo(404));
}

@Test
@Issue("JENKINS-38487")
public void startupShouldNotFailOnFailingOnlineListener() {
// We do nothing, FailingOnOnlineListener & JenkinsRule should cause the
// boot failure if the issue is not fixed.
}

@TestExtension(value = "startupShouldNotFailOnFailingOnlineListener")
public static final class FailingOnOnlineListener extends ComputerListener {

@Override
public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
throw new IOException("Something happened (the listener always throws this exception)");
}
}
}

0 comments on commit b104407

Please sign in to comment.