FS: write to JGit config in a background thread

Otherwise locking problems may ensue if the JGit config itself is
on a different file system. Since the internal is already updated,
it is not really important when exactly the value gets persisted.
By queueing up separate Runnables executed by a single thread we
avoid concurrent write access to the JGit config, and nested calls
to getFileStoreAttributes(Path) result in serialized attempts to
write.

The thread for writing the config must not be a daemon thread. If
it were, JVM shutdown might kill it anytime, which may lead to
the config not being written, or worse, a config.lock file being
left behind.

Bug: 566170
Change-Id: I07e3d4c5e029d3cec9ab5895002fc4e0c7948c40
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

1 files changed, 29 insertions, 4 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
index 4d7be0a4ca..c645ba927a 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -237,7 +237,8 @@ public abstract class FS {
 		private static final Executor FUTURE_RUNNER = new ThreadPoolExecutor(0,
 				5, 30L, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(),
 				runnable -> {
-					Thread t = new Thread(runnable, "FileStoreAttributeReader-" //$NON-NLS-1$
+					Thread t = new Thread(runnable,
+							"JGit-FileStoreAttributeReader-" //$NON-NLS-1$
 							+ threadNumber.getAndIncrement());
 					// Make sure these threads don't prevent application/JVM
 					// shutdown.
@@ -246,12 +247,34 @@ public abstract class FS {
 				});
 
 		/**
+		 * Use a separate executor with at most one thread to synchronize
+		 * writing to the config. We write asynchronously since the config
+		 * itself might be on a different file system, which might otherwise
+		 * lead to locking problems.
+		 * <p>
+		 * Writing the config must not use a daemon thread, otherwise we may
+		 * leave an inconsistent state on disk when the JVM shuts down. Use a
+		 * small keep-alive time to avoid delays on shut-down.
+		 * </p>
+		 */
+		private static final Executor SAVE_RUNNER = new ThreadPoolExecutor(0, 1,
+				1L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>(),
+				runnable -> {
+					Thread t = new Thread(runnable,
+							"JGit-FileStoreAttributeWriter-" //$NON-NLS-1$
+							+ threadNumber.getAndIncrement());
+					// Make sure these threads do finish
+					t.setDaemon(false);
+					return t;
+				});
+
+		/**
 		 * Whether FileStore attributes should be determined asynchronously
 		 *
 		 * @param async
 		 *            whether FileStore attributes should be determined
-		 *            asynchronously. If false access to cached attributes may block
-		 *            for some seconds for the first call per FileStore
+		 *            asynchronously. If false access to cached attributes may
+		 *            block for some seconds for the first call per FileStore
 		 * @since 5.6.2
 		 */
 		public static void setBackground(boolean async) {
@@ -371,7 +394,9 @@ public abstract class FS {
 									if (LOG.isDebugEnabled()) {
 										LOG.debug(c.toString());
 									}
-									saveToConfig(s, c);
+									FileStoreAttributes newAttrs = c;
+									SAVE_RUNNER.execute(
+											() -> saveToConfig(s, newAttrs));
 								}
 								attributes = Optional.of(c);
 							} finally {