Merge branch 'stable-5.6' into stable-5.7
* stable-5.6:
LockFile: create OutputStream only when needed
Change-Id: I7c0e37d2cee0923662a7e39df5a802a84c017e4f
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
index 0f93749..509935d 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012, GitHub Inc. and others
+ * Copyright (C) 2012, 2021 GitHub Inc. and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -9,10 +9,16 @@
*/
package org.eclipse.jgit.internal.storage.file;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import java.io.File;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+
import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.api.errors.JGitInternalException;
import org.eclipse.jgit.errors.LockFailedException;
@@ -49,4 +55,149 @@
}
}
}
+
+ @Test
+ public void testLockTwice() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ lock.write("other".getBytes(StandardCharsets.US_ASCII));
+ lock.commit();
+ assertFalse(lock.isLocked());
+ checkFile(f, "other");
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("second".getBytes(StandardCharsets.US_ASCII));
+ }
+ lock.commit();
+ assertFalse(lock.isLocked());
+ checkFile(f, "second");
+ }
+
+ @Test
+ public void testLockTwiceUnlock() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ lock.write("other".getBytes(StandardCharsets.US_ASCII));
+ lock.unlock();
+ assertFalse(lock.isLocked());
+ checkFile(f, "content");
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("second".getBytes(StandardCharsets.US_ASCII));
+ }
+ lock.commit();
+ assertFalse(lock.isLocked());
+ checkFile(f, "second");
+ }
+
+ @Test
+ public void testLockWriteTwiceThrows1() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ lock.write("other".getBytes(StandardCharsets.US_ASCII));
+ assertThrows(Exception.class,
+ () -> lock.write("second".getBytes(StandardCharsets.US_ASCII)));
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockWriteTwiceThrows2() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("other".getBytes(StandardCharsets.US_ASCII));
+ }
+ assertThrows(Exception.class,
+ () -> lock.write("second".getBytes(StandardCharsets.US_ASCII)));
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockWriteTwiceThrows3() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ lock.write("other".getBytes(StandardCharsets.US_ASCII));
+ assertThrows(Exception.class, () -> {
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("second".getBytes(StandardCharsets.US_ASCII));
+ }
+ });
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockWriteTwiceThrows4() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("other".getBytes(StandardCharsets.US_ASCII));
+ }
+ assertThrows(Exception.class, () -> {
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("second".getBytes(StandardCharsets.US_ASCII));
+ }
+ });
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockUnclosedCommitThrows() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ try (OutputStream out = lock.getOutputStream()) {
+ out.write("other".getBytes(StandardCharsets.US_ASCII));
+ assertThrows(Exception.class, () -> lock.commit());
+ }
+ }
+
+ @Test
+ public void testLockNested() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ assertThrows(IllegalStateException.class, () -> lock.lock());
+ assertTrue(lock.isLocked());
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockHeld() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lock());
+ assertTrue(lock.isLocked());
+ LockFile lock2 = new LockFile(f);
+ assertFalse(lock2.lock());
+ assertFalse(lock2.isLocked());
+ assertTrue(lock.isLocked());
+ lock.unlock();
+ }
+
+ @Test
+ public void testLockForAppend() throws Exception {
+ File f = writeTrashFile("somefile", "content");
+ LockFile lock = new LockFile(f);
+ assertTrue(lock.lockForAppend());
+ assertTrue(lock.isLocked());
+ lock.write("other".getBytes(StandardCharsets.US_ASCII));
+ lock.commit();
+ assertFalse(lock.isLocked());
+ checkFile(f, "contentother");
+ }
}
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 054a6ef..424bff0 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -410,11 +410,18 @@
localObjectsIncomplete=Local objects incomplete.
localRefIsMissingObjects=Local ref {0} is missing object(s).
localRepository=local repository
+lockAlreadyHeld=Lock on {0} already held
lockCountMustBeGreaterOrEqual1=lockCount must be >= 1
lockError=lock error: {0}
lockFailedRetry=locking {0} failed after {1} retries
lockOnNotClosed=Lock on {0} not closed.
lockOnNotHeld=Lock on {0} not held.
+lockStreamClosed=Output to lock on {0} already closed
+lockStreamMultiple=Output to lock on {0} already opened
+logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}.
+logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement.
+logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}.
+logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {}
maxCountMustBeNonNegative=max count must be >= 0
mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2}
mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 5b08e00..fd17d32 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -1,6 +1,6 @@
/*
* Copyright (C) 2010, 2013 Sasa Zivkov <sasa.zivkov@sap.com>
- * Copyright (C) 2012, Research In Motion Limited and others
+ * Copyright (C) 2012, 2021 Research In Motion Limited and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -439,10 +439,17 @@
/***/ public String localRefIsMissingObjects;
/***/ public String localRepository;
/***/ public String lockCountMustBeGreaterOrEqual1;
+ /***/ public String lockAlreadyHeld;
/***/ public String lockError;
/***/ public String lockFailedRetry;
/***/ public String lockOnNotClosed;
/***/ public String lockOnNotHeld;
+ /***/ public String lockStreamClosed;
+ /***/ public String lockStreamMultiple;
+ /***/ public String logInconsistentFiletimeDiff;
+ /***/ public String logLargerFiletimeDiff;
+ /***/ public String logSmallerFiletime;
+ /***/ public String logXDGConfigHomeInvalid;
/***/ public String maxCountMustBeNonNegative;
/***/ public String mergeConflictOnNonNoteEntries;
/***/ public String mergeConflictOnNotes;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java
index 2e0a6da..f57581a 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java
@@ -1,6 +1,6 @@
/*
* Copyright (C) 2007, Robin Rosenberg <robin.rosenberg@dewire.com>
- * Copyright (C) 2006-2008, Shawn O. Pearce <spearce@spearce.org> and others
+ * Copyright (C) 2006-2021, Shawn O. Pearce <spearce@spearce.org> and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -96,11 +96,15 @@
private boolean haveLck;
- FileOutputStream os;
+ private FileOutputStream os;
private boolean needSnapshot;
- boolean fsync;
+ private boolean fsync;
+
+ private boolean isAppend;
+
+ private boolean written;
private FileSnapshot commitSnapshot;
@@ -127,6 +131,10 @@
* does not hold the lock.
*/
public boolean lock() throws IOException {
+ if (haveLck) {
+ throw new IllegalStateException(
+ MessageFormat.format(JGitText.get().lockAlreadyHeld, ref));
+ }
FileUtils.mkdirs(lck.getParentFile(), true);
try {
token = FS.DETECTED.createNewFileAtomic(lck);
@@ -134,18 +142,15 @@
LOG.error(JGitText.get().failedCreateLockFile, lck, e);
throw e;
}
- if (token.isCreated()) {
+ boolean obtainedLock = token.isCreated();
+ if (obtainedLock) {
haveLck = true;
- try {
- os = new FileOutputStream(lck);
- } catch (IOException ioe) {
- unlock();
- throw ioe;
- }
+ isAppend = false;
+ written = false;
} else {
closeToken();
}
- return haveLck;
+ return obtainedLock;
}
/**
@@ -158,12 +163,24 @@
* does not hold the lock.
*/
public boolean lockForAppend() throws IOException {
- if (!lock())
+ if (!lock()) {
return false;
+ }
copyCurrentContent();
+ isAppend = true;
+ written = false;
return true;
}
+ // For tests only
+ boolean isLocked() {
+ return haveLck;
+ }
+
+ private FileOutputStream getStream() throws IOException {
+ return new FileOutputStream(lck, isAppend);
+ }
+
/**
* Copy the current file content into the temporary file.
* <p>
@@ -185,32 +202,31 @@
*/
public void copyCurrentContent() throws IOException {
requireLock();
- try {
+ try (FileOutputStream out = getStream()) {
try (FileInputStream fis = new FileInputStream(ref)) {
if (fsync) {
FileChannel in = fis.getChannel();
long pos = 0;
long cnt = in.size();
while (0 < cnt) {
- long r = os.getChannel().transferFrom(in, pos, cnt);
+ long r = out.getChannel().transferFrom(in, pos, cnt);
pos += r;
cnt -= r;
}
} else {
final byte[] buf = new byte[2048];
int r;
- while ((r = fis.read(buf)) >= 0)
- os.write(buf, 0, r);
+ while ((r = fis.read(buf)) >= 0) {
+ out.write(buf, 0, r);
}
}
} catch (FileNotFoundException fnfe) {
if (ref.exists()) {
- unlock();
throw fnfe;
}
// Don't worry about a file that doesn't exist yet, it
// conceptually has no current content to copy.
- //
+ }
} catch (IOException | RuntimeException | Error ioe) {
unlock();
throw ioe;
@@ -253,18 +269,22 @@
*/
public void write(byte[] content) throws IOException {
requireLock();
- try {
+ try (FileOutputStream out = getStream()) {
+ if (written) {
+ throw new IOException(MessageFormat
+ .format(JGitText.get().lockStreamClosed, ref));
+ }
if (fsync) {
- FileChannel fc = os.getChannel();
+ FileChannel fc = out.getChannel();
ByteBuffer buf = ByteBuffer.wrap(content);
- while (0 < buf.remaining())
+ while (0 < buf.remaining()) {
fc.write(buf);
+ }
fc.force(true);
} else {
- os.write(content);
+ out.write(content);
}
- os.close();
- os = null;
+ written = true;
} catch (IOException | RuntimeException | Error ioe) {
unlock();
throw ioe;
@@ -283,36 +303,68 @@
public OutputStream getOutputStream() {
requireLock();
- final OutputStream out;
- if (fsync)
- out = Channels.newOutputStream(os.getChannel());
- else
- out = os;
+ if (written || os != null) {
+ throw new IllegalStateException(MessageFormat
+ .format(JGitText.get().lockStreamMultiple, ref));
+ }
return new OutputStream() {
+
+ private OutputStream out;
+
+ private boolean closed;
+
+ private OutputStream get() throws IOException {
+ if (written) {
+ throw new IOException(MessageFormat
+ .format(JGitText.get().lockStreamMultiple, ref));
+ }
+ if (out == null) {
+ os = getStream();
+ if (fsync) {
+ out = Channels.newOutputStream(os.getChannel());
+ } else {
+ out = os;
+ }
+ }
+ return out;
+ }
+
@Override
public void write(byte[] b, int o, int n)
throws IOException {
- out.write(b, o, n);
+ get().write(b, o, n);
}
@Override
public void write(byte[] b) throws IOException {
- out.write(b);
+ get().write(b);
}
@Override
public void write(int b) throws IOException {
- out.write(b);
+ get().write(b);
}
@Override
public void close() throws IOException {
+ if (closed) {
+ return;
+ }
+ closed = true;
try {
- if (fsync)
+ if (written) {
+ throw new IOException(MessageFormat
+ .format(JGitText.get().lockStreamClosed, ref));
+ }
+ if (out != null) {
+ if (fsync) {
os.getChannel().force(true);
+ }
out.close();
os = null;
+ }
+ written = true;
} catch (IOException | RuntimeException | Error ioe) {
unlock();
throw ioe;
@@ -322,7 +374,7 @@
}
void requireLock() {
- if (os == null) {
+ if (!haveLck) {
unlock();
throw new IllegalStateException(MessageFormat.format(JGitText.get().lockOnNotHeld, ref));
}
@@ -411,6 +463,8 @@
try {
FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE);
haveLck = false;
+ isAppend = false;
+ written = false;
closeToken();
return true;
} catch (IOException e) {
@@ -497,6 +551,8 @@
closeToken();
}
}
+ isAppend = false;
+ written = false;
}
/** {@inheritDoc} */
