From 4a708e2b0a747fbc24300ba862a8fe8580620065 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 31 Aug 2023 09:44:22 +0200 Subject: [PATCH 1/6] In case an extension of Slave overrides readResolve but does not call its super implementation recover from it, and issue a warning to raise a bug against the impacted plugin. --- core/src/main/java/hudson/model/Slave.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java index da5195a963cf..9fc25d1c9889 100644 --- a/core/src/main/java/hudson/model/Slave.java +++ b/core/src/main/java/hudson/model/Slave.java @@ -311,6 +311,10 @@ public DescribableList, NodePropertyDescriptor> getNodePropertie @DataBoundSetter public void setNodeProperties(List> properties) throws IOException { + if (nodeProperties == null) { + warnPlugin(); + nodeProperties = new DescribableList<>(this); + } nodeProperties.replaceBy(properties); } @@ -346,9 +350,17 @@ private void _setLabelString(String labelString) { @Override protected Set getLabelAtomSet() { + if (labelAtomSet == null) { + warnPlugin(); + this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label)); + } return labelAtomSet; } + private void warnPlugin() { + LOGGER.log(Level.WARNING, () -> getClass().getName() + " or one of its superclass overrides readResolve() without calling super implementation. Please file an issue against the plugin owning it : " + Jenkins.get().getPluginManager().whichPlugin(getClass())); + } + @Override public Callable getClockDifferenceCallable() { return new GetClockDifference1(); From b1f86e4fefc9d1bc9210a39ba018c6573aa39a6a Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 31 Aug 2023 10:04:44 +0200 Subject: [PATCH 2/6] Add a test --- .../java/jenkins/model/NodesRestartTest.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 test/src/test/java/jenkins/model/NodesRestartTest.java diff --git a/test/src/test/java/jenkins/model/NodesRestartTest.java b/test/src/test/java/jenkins/model/NodesRestartTest.java new file mode 100644 index 000000000000..837bed98d718 --- /dev/null +++ b/test/src/test/java/jenkins/model/NodesRestartTest.java @@ -0,0 +1,47 @@ +package jenkins.model; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.hasSize; +import static org.junit.Assert.assertNotNull; + +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.model.Descriptor; +import hudson.model.Slave; +import hudson.slaves.ComputerLauncher; +import java.io.IOException; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.RestartableJenkinsRule; + +public class NodesRestartTest { + @Rule + public RestartableJenkinsRule s = new RestartableJenkinsRule(); + + // The point of this implementation is to override readResolve so that Slave#readResolve doesn't get called. + public static class DummyDumbSlave extends Slave { + public DummyDumbSlave(@NonNull String name, String remoteFS, ComputerLauncher launcher) throws Descriptor.FormException, IOException { + super(name, remoteFS, launcher); + } + + @Override + protected Object readResolve() { + return this; + } + } + + @Test + public void checkNodeRestart() throws Exception { + s.then(r -> { + assertThat(r.jenkins.getNodes(), hasSize(0)); + var node = new DummyDumbSlave("my-node", "temp", r.createComputerLauncher(null)); + r.jenkins.addNode(node); + assertThat(r.jenkins.getNodes(), hasSize(1)); + }); + s.then(r -> { + assertThat(r.jenkins.getNodes(), hasSize(1)); + var node = r.jenkins.getNode("my-node"); + assertNotNull(node.getNodeProperties()); + assertNotNull(node.getAssignedLabels()); + }); + } +} From 7cbb82cdc29b07b49992a9f3ee39a15a0f8e45c0 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 31 Aug 2023 13:24:13 +0200 Subject: [PATCH 3/6] Fix spotbugs/checkstyle --- core/src/main/java/hudson/model/Slave.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java index 9fc25d1c9889..86a44c44a559 100644 --- a/core/src/main/java/hudson/model/Slave.java +++ b/core/src/main/java/hudson/model/Slave.java @@ -345,7 +345,7 @@ private void _setLabelString(String labelString) { this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label)); } - @NonNull + @CheckForNull // should be @NonNull, but we've seen plugins overriding readResolve() without calling super. private transient Set labelAtomSet; @Override @@ -358,7 +358,8 @@ protected Set getLabelAtomSet() { } private void warnPlugin() { - LOGGER.log(Level.WARNING, () -> getClass().getName() + " or one of its superclass overrides readResolve() without calling super implementation. Please file an issue against the plugin owning it : " + Jenkins.get().getPluginManager().whichPlugin(getClass())); + LOGGER.log(Level.WARNING, () -> getClass().getName() + " or one of its superclass overrides readResolve() without calling super implementation." + + "Please file an issue against the plugin owning it : " + Jenkins.get().getPluginManager().whichPlugin(getClass())); } @Override From 9a9cac88d36de2e7e47aa9999cde7a14bffe1883 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 31 Aug 2023 13:56:57 +0200 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com> --- core/src/main/java/hudson/model/Slave.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java index 86a44c44a559..fe69bac0af93 100644 --- a/core/src/main/java/hudson/model/Slave.java +++ b/core/src/main/java/hudson/model/Slave.java @@ -359,7 +359,7 @@ protected Set getLabelAtomSet() { private void warnPlugin() { LOGGER.log(Level.WARNING, () -> getClass().getName() + " or one of its superclass overrides readResolve() without calling super implementation." + - "Please file an issue against the plugin owning it : " + Jenkins.get().getPluginManager().whichPlugin(getClass())); + "Please file an issue against the plugin implementing it: " + Jenkins.get().getPluginManager().whichPlugin(getClass())); } @Override From 9680241ebbca4d6043e43187ba4f03366e8cb892 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 1 Sep 2023 09:44:28 +0200 Subject: [PATCH 5/6] Move test to JenkinsSessionRule --- test/src/test/java/jenkins/model/NodesRestartTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/src/test/java/jenkins/model/NodesRestartTest.java b/test/src/test/java/jenkins/model/NodesRestartTest.java index 837bed98d718..ada5e89f3482 100644 --- a/test/src/test/java/jenkins/model/NodesRestartTest.java +++ b/test/src/test/java/jenkins/model/NodesRestartTest.java @@ -11,11 +11,11 @@ import java.io.IOException; import org.junit.Rule; import org.junit.Test; -import org.jvnet.hudson.test.RestartableJenkinsRule; +import org.jvnet.hudson.test.JenkinsSessionRule; public class NodesRestartTest { @Rule - public RestartableJenkinsRule s = new RestartableJenkinsRule(); + public JenkinsSessionRule s = new JenkinsSessionRule(); // The point of this implementation is to override readResolve so that Slave#readResolve doesn't get called. public static class DummyDumbSlave extends Slave { @@ -30,7 +30,7 @@ protected Object readResolve() { } @Test - public void checkNodeRestart() throws Exception { + public void checkNodeRestart() throws Throwable { s.then(r -> { assertThat(r.jenkins.getNodes(), hasSize(0)); var node = new DummyDumbSlave("my-node", "temp", r.createComputerLauncher(null)); From 65ce7507550706816424b9e1c884563410ed5d36 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 1 Sep 2023 09:46:06 +0200 Subject: [PATCH 6/6] Rename class --- test/src/test/java/jenkins/model/NodesRestartTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/src/test/java/jenkins/model/NodesRestartTest.java b/test/src/test/java/jenkins/model/NodesRestartTest.java index ada5e89f3482..9c86fd8dedc4 100644 --- a/test/src/test/java/jenkins/model/NodesRestartTest.java +++ b/test/src/test/java/jenkins/model/NodesRestartTest.java @@ -18,8 +18,8 @@ public class NodesRestartTest { public JenkinsSessionRule s = new JenkinsSessionRule(); // The point of this implementation is to override readResolve so that Slave#readResolve doesn't get called. - public static class DummyDumbSlave extends Slave { - public DummyDumbSlave(@NonNull String name, String remoteFS, ComputerLauncher launcher) throws Descriptor.FormException, IOException { + public static class DummyAgent extends Slave { + public DummyAgent(@NonNull String name, String remoteFS, ComputerLauncher launcher) throws Descriptor.FormException, IOException { super(name, remoteFS, launcher); } @@ -33,7 +33,7 @@ protected Object readResolve() { public void checkNodeRestart() throws Throwable { s.then(r -> { assertThat(r.jenkins.getNodes(), hasSize(0)); - var node = new DummyDumbSlave("my-node", "temp", r.createComputerLauncher(null)); + var node = new DummyAgent("my-node", "temp", r.createComputerLauncher(null)); r.jenkins.addNode(node); assertThat(r.jenkins.getNodes(), hasSize(1)); });