motiejus
/
jgit
1
Fork 0
Code Releases Activity

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 <twolf@apache.org>
This commit is contained in:
Thomas Wolf 2023-10-14 23:25:19 +02:00
parent f6774fa8ee
commit 0f078da4e4
1 changed files with 71 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

122
org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
View File

@ -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}
* <p>
@ -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);