Fix concurrent read / write issue in LockFile on Windows

LockFile.commit fails if another thread concurrently reads
the base file. The problem is fixed by retrying the rename
operation if it fails.

Change-Id: I6bb76ea7f2e6e90e3ddc45f9dd4d69bd1b6fa1eb
Bug: 308506
Signed-off-by: Jens Baumgart <jens.baumgart@sap.com>
This commit is contained in:
Jens Baumgart 2010-07-26 10:18:47 +02:00 committed by Shawn O. Pearce
parent a00377a7e2
commit db82b8d7eb
25 changed files with 122 additions and 50 deletions

View File

@ -61,6 +61,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.storage.file.LockFile;
import org.eclipse.jgit.util.FS;
/**
* Manages the {@code .eclipse_iplog} file in a project.
@ -167,6 +168,9 @@ private List<Project> parseProjects(final Config cfg,
*
* @param file
* local file to update with current CQ records.
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
* @param base
* base https:// URL of the IPzilla server.
* @param username
@ -181,16 +185,16 @@ private List<Project> parseProjects(final Config cfg,
* the local file cannot be read, as it is not a valid
* configuration file format.
*/
public void syncCQs(File file, URL base, String username, String password)
throws IOException, ConfigInvalidException {
public void syncCQs(File file, FS fs, URL base, String username,
String password) throws IOException, ConfigInvalidException {
if (!file.getParentFile().exists())
file.getParentFile().mkdirs();
LockFile lf = new LockFile(file);
LockFile lf = new LockFile(file, fs);
if (!lf.lock())
throw new IOException(MessageFormat.format(IpLogText.get().cannotLock, file));
try {
FileBasedConfig cfg = new FileBasedConfig(file);
FileBasedConfig cfg = new FileBasedConfig(file, fs);
cfg.load();
loadFrom(cfg);

View File

@ -69,6 +69,7 @@
import org.eclipse.jgit.storage.file.FileRepository;
import org.eclipse.jgit.storage.file.WindowCache;
import org.eclipse.jgit.storage.file.WindowCacheConfig;
import org.eclipse.jgit.util.FS;
import org.eclipse.jgit.util.IO;
import org.eclipse.jgit.util.SystemReader;
@ -128,7 +129,7 @@ public void run() {
mockSystemReader = new MockSystemReader();
mockSystemReader.userGitConfig = new FileBasedConfig(new File(trash,
"usergitconfig"));
"usergitconfig"), FS.DETECTED);
ceilTestDirectories(getCeilings());
SystemReader.setInstance(mockSystemReader);

View File

@ -67,7 +67,7 @@ public MockSystemReader() {
init(Constants.GIT_AUTHOR_EMAIL_KEY);
init(Constants.GIT_COMMITTER_NAME_KEY);
init(Constants.GIT_COMMITTER_EMAIL_KEY);
userGitConfig = new FileBasedConfig(null) {
userGitConfig = new FileBasedConfig(null, null) {
@Override
public void load() throws IOException, ConfigInvalidException {
// Do nothing

View File

@ -654,7 +654,7 @@ private static File nameFor(ObjectDirectory odb, ObjectId name, String t) {
private void writeFile(final File p, final byte[] bin) throws IOException,
ObjectWritingException {
final LockFile lck = new LockFile(p);
final LockFile lck = new LockFile(p, db.getFS());
if (!lck.lock())
throw new ObjectWritingException("Can't write " + p);
try {

View File

@ -227,7 +227,7 @@ private void detachHead() throws IOException {
final ObjectId id = db.resolve(Constants.HEAD);
if (!ObjectId.isId(head) && id != null) {
final LockFile lf;
lf = new LockFile(new File(db.getDirectory(), Constants.HEAD));
lf = new LockFile(new File(db.getDirectory(), Constants.HEAD), db.getFS());
if (!lf.lock())
throw new IOException(MessageFormat.format(CLIText.get().cannotLock, Constants.HEAD));
lf.write(id);
@ -254,7 +254,7 @@ private void recreateRefs() throws Exception {
protected void writeFile(final String name, final byte[] content)
throws IOException {
final File file = new File(db.getDirectory(), name);
final LockFile lck = new LockFile(file);
final LockFile lck = new LockFile(file, db.getFS());
if (!lck.lock())
throw new ObjectWritingException(MessageFormat.format(CLIText.get().cantWrite, file));
try {

View File

@ -98,7 +98,7 @@ else if (version == null)
if (output != null) {
if (!output.getParentFile().exists())
output.getParentFile().mkdirs();
LockFile lf = new LockFile(output);
LockFile lf = new LockFile(output, db.getFS());
if (!lf.lock())
throw die(MessageFormat.format(CLIText.get().cannotLock, output));
try {

View File

@ -94,6 +94,6 @@ protected void run() throws Exception {
output = new File(db.getWorkTree(), IpLogMeta.IPLOG_CONFIG_FILE);
IpLogMeta meta = new IpLogMeta();
meta.syncCQs(output, ipzilla, username, password);
meta.syncCQs(output, db.getFS(), ipzilla, username, password);
}
}

View File

@ -103,7 +103,7 @@ public int parseArguments(final Parameters params) throws CmdLineException {
if (new File(name).isFile()) {
final DirCache dirc;
try {
dirc = DirCache.read(new File(name));
dirc = DirCache.read(new File(name), FS.DETECTED);
} catch (IOException e) {
throw new CmdLineException(MessageFormat.format(CLIText.get().notAnIndexFile, name), e);
}

View File

@ -63,7 +63,7 @@ public void testReadMissing_TempIndex() throws Exception {
final File idx = new File(db.getDirectory(), "tmp_index");
assertFalse(idx.exists());
final DirCache dc = DirCache.read(idx);
final DirCache dc = DirCache.read(idx, db.getFS());
assertNotNull(dc);
assertEquals(0, dc.getEntryCount());
}
@ -91,7 +91,7 @@ public void testLockMissing_TempIndex() throws Exception {
assertFalse(idx.exists());
assertFalse(lck.exists());
final DirCache dc = DirCache.lock(idx);
final DirCache dc = DirCache.lock(idx, db.getFS());
assertNotNull(dc);
assertFalse(idx.exists());
assertTrue(lck.exists());

View File

@ -58,6 +58,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.treewalk.TreeWalk;
import org.eclipse.jgit.util.FS;
import org.eclipse.jgit.util.JGitTestUtil;
public class DirCacheCGitCompatabilityTest extends LocalDiskRepositoryTestCase {
@ -65,7 +66,7 @@ public class DirCacheCGitCompatabilityTest extends LocalDiskRepositoryTestCase {
public void testReadIndex_LsFiles() throws Exception {
final Map<String, CGitIndexRecord> ls = readLsFiles();
final DirCache dc = new DirCache(index);
final DirCache dc = new DirCache(index, FS.DETECTED);
assertEquals(0, dc.getEntryCount());
dc.read();
assertEquals(ls.size(), dc.getEntryCount());
@ -79,7 +80,7 @@ public void testReadIndex_LsFiles() throws Exception {
public void testTreeWalk_LsFiles() throws Exception {
final Repository db = createBareRepository();
final Map<String, CGitIndexRecord> ls = readLsFiles();
final DirCache dc = new DirCache(index);
final DirCache dc = new DirCache(index, db.getFS());
assertEquals(0, dc.getEntryCount());
dc.read();
assertEquals(ls.size(), dc.getEntryCount());
@ -102,14 +103,16 @@ public void testTreeWalk_LsFiles() throws Exception {
}
public void testUnsupportedOptionalExtension() throws Exception {
final DirCache dc = new DirCache(pathOf("gitgit.index.ZZZZ"));
final DirCache dc = new DirCache(pathOf("gitgit.index.ZZZZ"),
FS.DETECTED);
dc.read();
assertEquals(1, dc.getEntryCount());
assertEquals("A", dc.getEntry(0).getPathString());
}
public void testUnsupportedRequiredExtension() throws Exception {
final DirCache dc = new DirCache(pathOf("gitgit.index.aaaa"));
final DirCache dc = new DirCache(pathOf("gitgit.index.aaaa"),
FS.DETECTED);
try {
dc.read();
fail("Cache loaded an unsupported extension");
@ -120,7 +123,8 @@ public void testUnsupportedRequiredExtension() throws Exception {
}
public void testCorruptChecksumAtFooter() throws Exception {
final DirCache dc = new DirCache(pathOf("gitgit.index.badchecksum"));
final DirCache dc = new DirCache(pathOf("gitgit.index.badchecksum"),
FS.DETECTED);
try {
dc.read();
fail("Cache loaded despite corrupt checksum");
@ -143,7 +147,7 @@ private static void assertEqual(final CGitIndexRecord c,
public void testReadIndex_DirCacheTree() throws Exception {
final Map<String, CGitIndexRecord> cList = readLsFiles();
final Map<String, CGitLsTreeRecord> cTree = readLsTree();
final DirCache dc = new DirCache(index);
final DirCache dc = new DirCache(index, FS.DETECTED);
assertEquals(0, dc.getEntryCount());
dc.read();
assertEquals(cList.size(), dc.getEntryCount());

View File

@ -555,13 +555,15 @@ public void testUpdateRefLockFailureLocked() throws IOException {
ObjectId pid = db.resolve("refs/heads/master^");
RefUpdate updateRef = db.updateRef("refs/heads/master");
updateRef.setNewObjectId(pid);
LockFile lockFile1 = new LockFile(new File(db.getDirectory(),"refs/heads/master"));
LockFile lockFile1 = new LockFile(new File(db.getDirectory(),
"refs/heads/master"), db.getFS());
try {
assertTrue(lockFile1.lock()); // precondition to test
Result update = updateRef.update();
assertEquals(Result.LOCK_FAILURE, update);
assertEquals(opid, db.resolve("refs/heads/master"));
LockFile lockFile2 = new LockFile(new File(db.getDirectory(),"refs/heads/master"));
LockFile lockFile2 = new LockFile(new File(db.getDirectory(),"refs/heads/master"),
db.getFS());
assertFalse(lockFile2.lock()); // was locked, still is
} finally {
lockFile1.unlock();
@ -699,7 +701,8 @@ public void tryRenameWhenLocked(String toLock, String fromName,
"logs/" + fromName).exists());
// "someone" has branch X locked
LockFile lockFile = new LockFile(new File(db.getDirectory(), toLock));
LockFile lockFile = new LockFile(new File(db.getDirectory(), toLock),
db.getFS());
try {
assertTrue(lockFile.lock());

View File

@ -53,6 +53,7 @@
import org.eclipse.jgit.lib.ConfigConstants;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.util.FS;
/**
* Tests for setting up the working directory when creating a Repository
@ -191,7 +192,8 @@ private void setWorkTree(File gitDir, File workTree) throws IOException,
private FileBasedConfig configFor(File gitDir) throws IOException,
ConfigInvalidException {
FileBasedConfig cfg = new FileBasedConfig(new File(gitDir, "config"));
File configPath = new File(gitDir, "config");
FileBasedConfig cfg = new FileBasedConfig(configPath, FS.DETECTED);
cfg.load();
return cfg;
}

View File

@ -315,7 +315,7 @@ public void test005_ReadSimpleConfig() {
public void test006_ReadUglyConfig() throws IOException,
ConfigInvalidException {
final File cfg = new File(db.getDirectory(), "config");
final FileBasedConfig c = new FileBasedConfig(cfg);
final FileBasedConfig c = new FileBasedConfig(cfg, db.getFS());
final FileWriter pw = new FileWriter(cfg);
final String configStr = " [core];comment\n\tfilemode = yes\n"
+ "[user]\n"

View File

@ -67,6 +67,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.storage.file.LockFile;
import org.eclipse.jgit.util.FS;
import org.eclipse.jgit.util.IO;
import org.eclipse.jgit.util.MutableInteger;
import org.eclipse.jgit.util.NB;
@ -129,7 +130,7 @@ static int cmp(final byte[] aPath, final int aLen, final byte[] bPath,
* memory).
*/
public static DirCache newInCore() {
return new DirCache(null);
return new DirCache(null, null);
}
/**
@ -141,6 +142,9 @@ public static DirCache newInCore() {
*
* @param indexLocation
* location of the index file on disk.
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
* @return a cache representing the contents of the specified index file (if
* it exists) or an empty cache if the file does not exist.
* @throws IOException
@ -149,9 +153,9 @@ public static DirCache newInCore() {
* the index file is using a format or extension that this
* library does not support.
*/
public static DirCache read(final File indexLocation)
public static DirCache read(final File indexLocation, final FS fs)
throws CorruptObjectException, IOException {
final DirCache c = new DirCache(indexLocation);
final DirCache c = new DirCache(indexLocation, fs);
c.read();
return c;
}
@ -161,11 +165,14 @@ public static DirCache read(final File indexLocation)
* <p>
* The new index will be locked and then read before it is returned to the
* caller. Read failures are reported as exceptions and therefore prevent
* the method from returning a partially populated index. On read failure,
* the method from returning a partially populated index. On read failure,
* the lock is released.
*
* @param indexLocation
* location of the index file on disk.
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
* @return a cache representing the contents of the specified index file (if
* it exists) or an empty cache if the file does not exist.
* @throws IOException
@ -175,9 +182,9 @@ public static DirCache read(final File indexLocation)
* the index file is using a format or extension that this
* library does not support.
*/
public static DirCache lock(final File indexLocation)
public static DirCache lock(final File indexLocation, final FS fs)
throws CorruptObjectException, IOException {
final DirCache c = new DirCache(indexLocation);
final DirCache c = new DirCache(indexLocation, fs);
if (!c.lock())
throw new IOException(MessageFormat.format(JGitText.get().cannotLock, indexLocation));
@ -215,6 +222,9 @@ public static DirCache lock(final File indexLocation)
/** Our active lock (if we hold it); null if we don't have it locked. */
private LockFile myLock;
/** file system abstraction **/
private final FS fs;
/**
* Create a new in-core index representation.
* <p>
@ -223,9 +233,13 @@ public static DirCache lock(final File indexLocation)
*
* @param indexLocation
* location of the index file on disk.
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
*/
public DirCache(final File indexLocation) {
public DirCache(final File indexLocation, final FS fs) {
liveFile = indexLocation;
this.fs = fs;
clear();
}
@ -429,7 +443,7 @@ private static boolean is_DIRC(final byte[] hdr) {
public boolean lock() throws IOException {
if (liveFile == null)
throw new IOException(JGitText.get().dirCacheDoesNotHaveABackingFile);
final LockFile tmp = new LockFile(liveFile);
final LockFile tmp = new LockFile(liveFile, fs);
if (tmp.lock()) {
tmp.setNeedStatInformation(true);
myLock = tmp;

View File

@ -555,7 +555,7 @@ protected Config loadConfig() throws IOException {
// repository and not inherited from other files.
//
File path = safeFS().resolve(getGitDir(), "config");
FileBasedConfig cfg = new FileBasedConfig(path);
FileBasedConfig cfg = new FileBasedConfig(path, safeFS());
try {
cfg.load();
} catch (ConfigInvalidException err) {

View File

@ -930,7 +930,7 @@ public File getIndexFile() throws NoWorkTreeException {
*/
public DirCache readDirCache() throws NoWorkTreeException,
CorruptObjectException, IOException {
return DirCache.read(getIndexFile());
return DirCache.read(getIndexFile(), getFS());
}
/**
@ -954,7 +954,7 @@ public DirCache readDirCache() throws NoWorkTreeException,
*/
public DirCache lockDirCache() throws NoWorkTreeException,
CorruptObjectException, IOException {
return DirCache.lock(getIndexFile());
return DirCache.lock(getIndexFile(), getFS());
}
static byte[] gitInternalSlash(byte[] bytes) {

View File

@ -59,6 +59,7 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.StoredConfig;
import org.eclipse.jgit.util.FS;
import org.eclipse.jgit.util.IO;
import org.eclipse.jgit.util.RawParseUtils;
@ -68,15 +69,19 @@
public class FileBasedConfig extends StoredConfig {
private final File configFile;
private volatile long lastModified;
private final FS fs;
/**
* Create a configuration with no default fallback.
*
* @param cfgLocation
* the location of the configuration file on the file system
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
*/
public FileBasedConfig(File cfgLocation) {
this(null, cfgLocation);
public FileBasedConfig(File cfgLocation, FS fs) {
this(null, cfgLocation, fs);
}
/**
@ -86,10 +91,14 @@ public FileBasedConfig(File cfgLocation) {
* the base configuration file
* @param cfgLocation
* the location of the configuration file on the file system
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
*/
public FileBasedConfig(Config base, File cfgLocation) {
public FileBasedConfig(Config base, File cfgLocation, FS fs) {
super(base);
configFile = cfgLocation;
this.fs = fs;
}
/** @return location of the configuration file on disk */
@ -138,7 +147,7 @@ public void load() throws IOException, ConfigInvalidException {
*/
public void save() throws IOException {
final byte[] out = Constants.encode(toText());
final LockFile lf = new LockFile(getFile());
final LockFile lf = new LockFile(getFile(), fs);
if (!lf.lock())
throw new IOException(MessageFormat.format(JGitText.get().cannotLockFile, getFile()));
try {

View File

@ -136,7 +136,9 @@ public FileRepository(final BaseRepositoryBuilder options) throws IOException {
super(options);
userConfig = SystemReader.getInstance().openUserConfig(getFS());
repoConfig = new FileBasedConfig(userConfig, getFS().resolve(getDirectory(), "config"));
repoConfig = new FileBasedConfig(userConfig, //
getFS().resolve(getDirectory(), "config"), //
getFS());
loadUserConfig();
loadRepoConfig();

View File

@ -59,6 +59,7 @@
import org.eclipse.jgit.JGitText;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.util.FS;
/**
* Git style file locking and replacement.
@ -94,15 +95,21 @@ public boolean accept(File dir, String name) {
private long commitLastModified;
private final FS fs;
/**
* Create a new lock for any file.
*
* @param f
* the file that will be locked.
* @param fs
* the file system abstraction which will be necessary to perform
* certain file system operations.
*/
public LockFile(final File f) {
public LockFile(final File f, FS fs) {
ref = f;
lck = new File(ref.getParentFile(), ref.getName() + SUFFIX);
this.fs = fs;
}
/**
@ -393,13 +400,32 @@ public boolean commit() {
saveStatInformation();
if (lck.renameTo(ref))
return true;
if (!ref.exists() || ref.delete())
if (!ref.exists() || deleteRef())
if (lck.renameTo(ref))
return true;
unlock();
return false;
}
private boolean deleteRef() {
if (!fs.retryFailedLockFileCommit())
return ref.delete();
// File deletion fails on windows if another thread is
// concurrently reading the same file. So try a few times.
//
for (int attempts = 0; attempts < 10; attempts++) {
if (ref.delete())
return true;
try {
Thread.sleep(100);
} catch (InterruptedException e) {
return false;
}
}
return false;
}
private void saveStatInformation() {
if (needStatInformation)
commitLastModified = lck.lastModified();

View File

@ -47,21 +47,26 @@
import java.io.IOException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.util.FS;
/** Keeps track of a {@link PackFile}'s associated <code>.keep</code> file. */
public class PackLock {
private final File keepFile;
private final FS fs;
/**
* Create a new lock for a pack file.
*
* @param packFile
* location of the <code>pack-*.pack</code> file.
* @param fs
* the filesystem abstraction used by the repository.
*/
public PackLock(final File packFile) {
public PackLock(final File packFile, final FS fs) {
final File p = packFile.getParentFile();
final String n = packFile.getName();
keepFile = new File(p, n.substring(0, n.length() - 5) + ".keep");
this.fs = fs;
}
/**
@ -78,7 +83,7 @@ public boolean lock(String msg) throws IOException {
return false;
if (!msg.endsWith("\n"))
msg += "\n";
final LockFile lf = new LockFile(keepFile);
final LockFile lf = new LockFile(keepFile, fs);
if (!lf.lock())
return false;
lf.write(Constants.encode(msg));

View File

@ -512,7 +512,8 @@ void delete(RefDirectoryUpdate update) throws IOException {
// we don't miss an edit made externally.
final PackedRefList packed = getPackedRefs();
if (packed.contains(name)) {
LockFile lck = new LockFile(packedRefsFile);
LockFile lck = new LockFile(packedRefsFile,
update.getRepository().getFS());
if (!lck.lock())
throw new IOException(MessageFormat.format(
JGitText.get().cannotLockFile, packedRefsFile));

View File

@ -79,7 +79,7 @@ protected boolean tryLock(boolean deref) throws IOException {
if (deref)
dst = dst.getLeaf();
String name = dst.getName();
lock = new LockFile(database.fileFor(name));
lock = new LockFile(database.fileFor(name), getRepository().getFS());
if (lock.lock()) {
dst = database.getRef(name);
setOldObjectId(dst != null ? dst.getObjectId() : null);

View File

@ -279,7 +279,8 @@ private void updateFETCH_HEAD(final FetchResult result) throws IOException {
File meta = transport.local.getDirectory();
if (meta == null)
return;
final LockFile lock = new LockFile(new File(meta, "FETCH_HEAD"));
final LockFile lock = new LockFile(new File(meta, "FETCH_HEAD"),
transport.local.getFS());
try {
if (lock.lock()) {
final Writer w = new OutputStreamWriter(lock.getOutputStream());

View File

@ -1101,7 +1101,7 @@ public PackLock renameAndOpenPack(final String lockMessage)
final File packDir = new File(repo.getObjectsDirectory(), "pack");
final File finalPack = new File(packDir, "pack-" + name + ".pack");
final File finalIdx = new File(packDir, "pack-" + name + ".idx");
final PackLock keep = new PackLock(finalPack);
final PackLock keep = new PackLock(finalPack, repo.getFS());
if (!packDir.exists() && !packDir.mkdir() && !packDir.exists()) {
// The objects/pack directory isn't present, and we are unable

View File

@ -74,7 +74,7 @@ public String getProperty(String key) {
public FileBasedConfig openUserConfig(FS fs) {
final File home = fs.userHome();
return new FileBasedConfig(new File(home, ".gitconfig"));
return new FileBasedConfig(new File(home, ".gitconfig"), fs);
}
public String getHostname() {