From 0f078da4e45c90e1800fd16ee75c2934b4d58932 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 14 Oct 2023 23:25:19 +0200 Subject: [PATCH] FileBasedConfig: in-process synchronization for load() and save() On Windows reading and replacing a file via renaming concurrently may fail either in the reader or in the thread renaming the file. For renaming, FileUtils.rename() has a last-case fallback in which it deletes the target file before attempting the rename. If a reader reads at that moment, it will produce an empty config, and the snapshot and hash may be wrong because the concurrently running save() may set them. It's not really possible to do all this in a thread-safe manner without some synchronization. Add a read-write lock to synchronize readers and writers to avoid at least that JGit steps on its own feet. Bug: 451508 Change-Id: I7e5f0f26e02f34ba02dc925a445044d3e21389b4 Signed-off-by: Thomas Wolf --- .../jgit/storage/file/FileBasedConfig.java | 122 ++++++++++-------- 1 file changed, 71 insertions(+), 51 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java index c80505d70..3b7c02eca 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java @@ -20,9 +20,11 @@ import java.io.ByteArrayOutputStream; import java.io.File; +import java.io.FileNotFoundException; import java.io.IOException; import java.text.MessageFormat; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.LockFailedException; @@ -47,6 +49,9 @@ public class FileBasedConfig extends StoredConfig { private final FS fs; + // In-process synchronization between load() and save(). + private final ReentrantReadWriteLock lock; + private boolean utf8Bom; private volatile FileSnapshot snapshot; @@ -55,6 +60,7 @@ public class FileBasedConfig extends StoredConfig { private AtomicBoolean exists = new AtomicBoolean(); + /** * Create a configuration with no default fallback. * @@ -85,6 +91,7 @@ public FileBasedConfig(Config base, File cfgLocation, FS fs) { this.fs = fs; this.snapshot = FileSnapshot.DIRTY; this.hash = ObjectId.zeroId(); + this.lock = new ReentrantReadWriteLock(false); } @Override @@ -116,35 +123,9 @@ boolean exists() { */ @Override public void load() throws IOException, ConfigInvalidException { + lock.readLock().lock(); try { - Boolean wasRead = FileUtils.readWithRetries(getFile(), f -> { - final FileSnapshot oldSnapshot = snapshot; - final FileSnapshot newSnapshot; - // don't use config in this snapshot to avoid endless recursion - newSnapshot = FileSnapshot.saveNoConfig(f); - final byte[] in = IO.readFully(f); - final ObjectId newHash = hash(in); - if (hash.equals(newHash)) { - if (oldSnapshot.equals(newSnapshot)) { - oldSnapshot.setClean(newSnapshot); - } else { - snapshot = newSnapshot; - } - } else { - final String decoded; - if (isUtf8(in)) { - decoded = RawParseUtils.decode(UTF_8, - in, 3, in.length); - utf8Bom = true; - } else { - decoded = RawParseUtils.decode(in); - } - fromText(decoded); - snapshot = newSnapshot; - hash = newHash; - } - return Boolean.TRUE; - }); + Boolean wasRead = FileUtils.readWithRetries(getFile(), this::load); if (wasRead == null) { clear(); snapshot = FileSnapshot.MISSING_FILE; @@ -155,9 +136,38 @@ public void load() throws IOException, ConfigInvalidException { } catch (Exception e) { throw new ConfigInvalidException(MessageFormat .format(JGitText.get().cannotReadFile, getFile()), e); + } finally { + lock.readLock().unlock(); } } + private Boolean load(File f) throws Exception { + FileSnapshot oldSnapshot = snapshot; + // don't use config in this snapshot to avoid endless recursion + FileSnapshot newSnapshot = FileSnapshot.saveNoConfig(f); + byte[] in = IO.readFully(f); + ObjectId newHash = hash(in); + if (hash.equals(newHash)) { + if (oldSnapshot.equals(newSnapshot)) { + oldSnapshot.setClean(newSnapshot); + } else { + snapshot = newSnapshot; + } + } else { + String decoded; + if (isUtf8(in)) { + decoded = RawParseUtils.decode(UTF_8, in, 3, in.length); + utf8Bom = true; + } else { + decoded = RawParseUtils.decode(in); + } + fromText(decoded); + snapshot = newSnapshot; + hash = newHash; + } + return Boolean.TRUE; + } + /** * {@inheritDoc} *

@@ -171,33 +181,41 @@ public void load() throws IOException, ConfigInvalidException { */ @Override public void save() throws IOException { - final byte[] out; - final String text = toText(); - if (utf8Bom) { - final ByteArrayOutputStream bos = new ByteArrayOutputStream(); - bos.write(0xEF); - bos.write(0xBB); - bos.write(0xBF); - bos.write(text.getBytes(UTF_8)); - out = bos.toByteArray(); - } else { - out = Constants.encode(text); - } - - final LockFile lf = new LockFile(getFile()); + lock.writeLock().lock(); try { - if (!lf.lock()) { - throw new LockFailedException(getFile()); + byte[] out; + String text = toText(); + if (utf8Bom) { + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + bos.write(0xEF); + bos.write(0xBB); + bos.write(0xBF); + bos.write(text.getBytes(UTF_8)); + out = bos.toByteArray(); + } else { + out = Constants.encode(text); } - lf.setNeedSnapshotNoConfig(true); - lf.write(out); - if (!lf.commit()) - throw new IOException(MessageFormat.format(JGitText.get().cannotCommitWriteTo, getFile())); + + LockFile lf = new LockFile(getFile()); + try { + if (!lf.lock()) { + throw new LockFailedException(getFile()); + } + lf.setNeedSnapshotNoConfig(true); + lf.write(out); + if (!lf.commit()) { + throw new IOException(MessageFormat.format( + JGitText.get().cannotCommitWriteTo, getFile())); + } + } finally { + lf.unlock(); + } + snapshot = lf.getCommitSnapshot(); + hash = hash(out); + exists.set(true); } finally { - lf.unlock(); + lock.writeLock().unlock(); } - snapshot = lf.getCommitSnapshot(); - hash = hash(out); // notify the listeners fireConfigChangedEvent(); } @@ -249,6 +267,8 @@ protected byte[] readIncludedConfig(String relPath) try { return IO.readFully(file); + } catch (FileNotFoundException e) { + return null; } catch (IOException ioe) { throw new ConfigInvalidException(MessageFormat .format(JGitText.get().cannotReadFile, relPath), ioe);