[JENKINS-45892] Enhanced diagnostics (#2997)

* [JENKINS-45892] Enhanced diagnostics. * Refined fix which should avoid a needless warning when called from MultiBranchProject.onLoad.
jenkinsci · Sep 3, 2017 · a032c59 · a032c59
1 parent 4d60ca6
commit a032c59
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 3 deletions.
diff --git a/core/src/main/java/hudson/XmlFile.java b/core/src/main/java/hudson/XmlFile.java
@@ -120,6 +120,7 @@ public final class XmlFile {
     private final XStream xs;
     private final File file;
     private static final Map<Object, Void> beingWritten = Collections.synchronizedMap(new IdentityHashMap<>());
+    private static final ThreadLocal<File> writing = new ThreadLocal<>();
 
     public XmlFile(File file) {
         this(DEFAULT_XSTREAM,file);
@@ -175,10 +176,12 @@ public void write( Object o ) throws IOException {
         try {
             w.write("<?xml version='1.0' encoding='UTF-8'?>\n");
             beingWritten.put(o, null);
+            writing.set(file);
             try {
                 xs.toXML(o, w);
             } finally {
                 beingWritten.remove(o);
+                writing.set(null);
             }
             w.commit();
         } catch(StreamException e) {
@@ -200,11 +203,11 @@ public void write( Object o ) throws IOException {
      * @since 2.74
      */
     public static Object replaceIfNotAtTopLevel(Object o, Supplier<Object> replacement) {
-        if (beingWritten.containsKey(o)) {
+        File currentlyWriting = writing.get();
+        if (beingWritten.containsKey(o) || currentlyWriting == null) {
             return o;
         } else {
-            // Unfortunately we cannot easily tell which XML file is actually being saved here, at least without implementing a custom Converter.
-            LOGGER.log(Level.WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", o);
+            LOGGER.log(Level.WARNING, "JENKINS-45892: reference to " + o + " being saved from unexpected " + currentlyWriting, new IllegalStateException());
             return replacement.get();
         }
     }

diff --git a/test/src/test/java/hudson/model/AbstractItem2Test.java b/test/src/test/java/hudson/model/AbstractItem2Test.java
@@ -23,20 +23,26 @@
  */
 package hudson.model;
 
+import hudson.XmlFile;
+import java.util.logging.Level;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.not;
 import org.junit.Test;
 import static org.junit.Assert.*;
 import org.junit.Rule;
 import org.junit.runners.model.Statement;
 import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.LoggerRule;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
 
 public class AbstractItem2Test {
 
     @Rule
     public RestartableJenkinsRule rr = new RestartableJenkinsRule();
 
+    @Rule
+    public LoggerRule logging = new LoggerRule().record(XmlFile.class, Level.WARNING).capture(100);
+
     @Issue("JENKINS-45892")
     @Test
     public void badSerialization() {
@@ -50,6 +56,8 @@ public void evaluate() throws Throwable {
                 String text = p2.getConfigFile().asString();
                 assertThat(text, not(containsString("<description>this is p1</description>")));
                 assertThat(text, containsString("<fullName>p1</fullName>"));
+                assertThat(logging.getMessages().toString(), containsString(p1.toString()));
+                assertThat(logging.getMessages().toString(), containsString(p2.getConfigFile().toString()));
             }
         });
         rr.addStep(new Statement() {