From b6ca1993b089fc35128009b39996ebd2ec9d79db Mon Sep 17 00:00:00 2001
From: Thomas Wolf <thomas.wolf@paranor.ch>
Date: Thu, 20 Aug 2020 22:43:13 +0200
Subject: [PATCH] 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>
---
 .../src/org/eclipse/jgit/util/FS.java         | 33 ++++++++++++++++---
 1 file 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 4d7be0a4c..c645ba927 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 static final class FileStoreAttributes {
 		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.
@@ -245,13 +246,35 @@ public static final class FileStoreAttributes {
 					return t;
 				});
 
+		/**
+		 * 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 @@ private static FileStoreAttributes getFileStoreAttributes(Path dir) {
 									if (LOG.isDebugEnabled()) {
 										LOG.debug(c.toString());
 									}
-									saveToConfig(s, c);
+									FileStoreAttributes newAttrs = c;
+									SAVE_RUNNER.execute(
+											() -> saveToConfig(s, newAttrs));
 								}
 								attributes = Optional.of(c);
 							} finally {