Permalink
Browse files

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>
  • Loading branch information...
1 parent a00377a commit db82b8d7eb646a2d31f1b4e52ab4a623743192e9 @jensbaumgart jensbaumgart committed with Jul 26, 2010
Showing with 122 additions and 50 deletions.
  1. +8 −4 org.eclipse.jgit.iplog/src/org/eclipse/jgit/iplog/IpLogMeta.java
  2. +2 −1 org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java
  3. +1 −1 org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/MockSystemReader.java
  4. +1 −1 org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java
  5. +2 −2 org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug/RebuildCommitGraph.java
  6. +1 −1 org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/eclipse/Iplog.java
  7. +1 −1 org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/eclipse/Ipzilla.java
  8. +1 −1 org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/AbstractTreeIteratorHandler.java
  9. +2 −2 org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCacheBasicTest.java
  10. +10 −6 org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCacheCGitCompatabilityTest.java
  11. +6 −3 org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefUpdateTest.java
  12. +3 −1 org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RepositorySetupWorkDirTest.java
  13. +1 −1 org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/T0003_Basic.java
  14. +22 −8 org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java
  15. +1 −1 org.eclipse.jgit/src/org/eclipse/jgit/lib/BaseRepositoryBuilder.java
  16. +2 −2 org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
  17. +13 −4 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
  18. +3 −1 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java
  19. +28 −2 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
  20. +7 −2 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java
  21. +2 −1 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
  22. +1 −1 org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryUpdate.java
  23. +2 −1 org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
  24. +1 −1 org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
  25. +1 −1 org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java
@@ -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 @@ void loadFrom(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 @@ void loadFrom(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);
@@ -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);
@@ -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
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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);
}
}
@@ -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);
}
@@ -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());
@@ -58,14 +58,15 @@
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 {
private final File index = pathOf("gitgit.index");
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());
@@ -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());
@@ -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;
}
@@ -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"
@@ -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;
@@ -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) {
@@ -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) {
Oops, something went wrong.

0 comments on commit db82b8d

Please sign in to comment.