Simplifying writeReplace methods by factoring common logic into XmlFile.replaceIfNotAtTopLevel.

core/src/main/java/hudson/XmlFile.java

@@ -50,8 +50,13 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.io.Serializable;
 import java.io.Writer;
 import java.io.StringWriter;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.apache.commons.io.IOUtils;
@@ -114,6 +119,7 @@ import org.apache.commons.io.IOUtils;
 public final class XmlFile {
     private final XStream xs;
     private final File file;
+    private static final Map<Object, Void> beingWritten = Collections.synchronizedMap(new IdentityHashMap<>());
 
     public XmlFile(File file) {
         this(DEFAULT_XSTREAM,file);
@@ -168,7 +174,12 @@ public final class XmlFile {
         AtomicFileWriter w = new AtomicFileWriter(file);
         try {
             w.write("<?xml version='1.0' encoding='UTF-8'?>\n");
-            xs.toXML(o,w);
+            beingWritten.put(o, null);
+            try {
+                xs.toXML(o, w);
+            } finally {
+                beingWritten.remove(o);
+            }
             w.commit();
         } catch(StreamException e) {
             throw new IOException(e);
@@ -177,6 +188,27 @@ public final class XmlFile {
         }
     }
 
+    /**
+     * Provides an XStream replacement for an object unless a call to {@link #write} is currently in progress.
+     * As per JENKINS-45892 this may be used by any class which expects to be written at top level to an XML file
+     * but which cannot safely be serialized as a nested object (for example, because it expects some {@code onLoad} hook):
+     * implement a {@code writeReplace} method delegating to this method.
+     * The replacement need not be {@link Serializable} since it is only necessary for use from XStream.
+     * @param o an object ({@code this} from {@code writeReplace})
+     * @param replacement a supplier of a safely serializable replacement object with a {@code readResolve} method
+     * @return {@code o}, if {@link #write} is being called on it, else the replacement
+     * @since FIXME
+     */
+    public static Object replaceIfNotAtTopLevel(Object o, Supplier<Object> replacement) {
+        if (beingWritten.containsKey(o)) {
+            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);
+            return replacement.get();
+        }
+    }
+
     public boolean exists() {
         return file.exists();
     }

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

@@ -44,12 +44,9 @@ import hudson.util.AlternativeUiTextProvider.Message;
 import hudson.util.AtomicFileWriter;
 import hudson.util.IOUtils;
 import hudson.util.Secret;
-import java.io.Serializable;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import jenkins.model.DirectlyModifiableTopLevelItemGroup;
 import jenkins.model.Jenkins;
@@ -514,16 +511,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet
      */
     public synchronized void save() throws IOException {
         if(BulkChange.contains(this))   return;
-        synchronized (saving) {
-            saving.add(this);
-        }
-        try {
-            getConfigFile().write(this);
-        } finally {
-            synchronized (saving) {
-                saving.remove(this);
-            }
-        }
+        getConfigFile().write(this);
         SaveableListener.fireOnChange(this, getConfigFile());
     }
 
@@ -531,20 +519,9 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet
         return Items.getConfigFile(this);
     }
 
-    private static final Set<AbstractItem> saving = new HashSet<>();
-
     private Object writeReplace() {
-        synchronized (saving) {
-            if (saving.contains(this)) {
-                return this;
-            } else {
-                LOGGER.log(Level.WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", this);
-                return new Replacer(this);
-            }
-        }
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
     }
-
-    /** Not {@link Serializable} for now, since we are only expecting to use this from XStream. */
     private static class Replacer {
         private final String fullName;
         Replacer(AbstractItem i) {

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

@@ -1922,16 +1922,7 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
      */
     public synchronized void save() throws IOException {
         if(BulkChange.contains(this))   return;
-        synchronized (saving) {
-            saving.add(this);
-        }
-        try {
-            getDataFile().write(this);
-        } finally {
-            synchronized (saving) {
-                saving.remove(this);
-            }
-        }
+        getDataFile().write(this);
         SaveableListener.fireOnChange(this, getDataFile());
     }
 
@@ -1939,21 +1930,9 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
         return new XmlFile(XSTREAM,new File(getRootDir(),"build.xml"));
     }
 
-    private static final Set<Run<?, ?>> saving = new HashSet<>();
-
     private Object writeReplace() {
-        synchronized (saving) {
-            if (saving.contains(this)) {
-                return this;
-            } else {
-                // Unfortunately we cannot easily tell which XML file is actually being saved here, at least without implementing a custom Converter.
-                LOGGER.log(WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", this);
-                return new Replacer(this);
-            }
-        }
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
     }
-
-    /** Not {@link Serializable} for now, since we are only expecting to use this from XStream. */
     private static class Replacer {
         private final String id;
         Replacer(Run<?, ?> r) {

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

@@ -50,7 +50,6 @@ import hudson.util.XStream2;
 import java.io.File;
 import java.io.FileFilter;
 import java.io.IOException;
-import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -732,33 +731,13 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
             throw FormValidation.error(Messages.User_IllegalFullname(fullName));
         }
         if(BulkChange.contains(this))   return;
-        synchronized (saving) {
-            saving.add(this);
-        }
-        try {
-            getConfigFile().write(this);
-        } finally {
-            synchronized (saving) {
-                saving.remove(this);
-            }
-        }
+        getConfigFile().write(this);
         SaveableListener.fireOnChange(this, getConfigFile());
     }
 
-    private static final Set<User> saving = new HashSet<>();
-
     private Object writeReplace() {
-        synchronized (saving) {
-            if (saving.contains(this)) {
-                return this;
-            } else {
-                LOGGER.log(Level.WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", this);
-                return new Replacer(this);
-            }
-        }
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
     }
-
-    /** Not {@link Serializable} for now, since we are only expecting to use this from XStream. */
     private static class Replacer {
         private final String id;
         Replacer(User u) {