Merge pull request #8448 from Vlatombe/labelAtomSet-robustness

Label parsing robustness fix

core/src/main/java/hudson/model/Slave.java

@@ -311,6 +311,10 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getNodePropertie
 
     @DataBoundSetter
     public void setNodeProperties(List<? extends NodeProperty<?>> properties) throws IOException {
+        if (nodeProperties == null) {
+            warnPlugin();
+            nodeProperties = new DescribableList<>(this);
+        }
         nodeProperties.replaceBy(properties);
     }
 
@@ -341,14 +345,23 @@ 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<LabelAtom> labelAtomSet;
 
     @Override
     protected Set<LabelAtom> 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 implementing it: " + Jenkins.get().getPluginManager().whichPlugin(getClass()));
+    }
+
     @Override
     public Callable<ClockDifference, IOException> getClockDifferenceCallable() {
         return new GetClockDifference1();

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.JenkinsSessionRule;
+
+public class NodesRestartTest {
+    @Rule
+    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 DummyAgent extends Slave {
+        public DummyAgent(@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 Throwable {
+        s.then(r -> {
+            assertThat(r.jenkins.getNodes(), hasSize(0));
+            var node = new DummyAgent("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());
+        });
+    }
+}