Revert "RefDirectory: Throw exception if CAS of packed ref list fails"
This reverts commit9c33f7364d
. Reason for revert: This change was based on the false claim that the packedrefs file lock is held while the CAS is being done, but it is actually released before the CAS (the in memory lock is still held, however that does not prevent external actors from updating the packedrefs files and then another thread from subsequently re-reading it and updating the in memory packedRefList). Although reverting this change can cause the CAS to fail, it should not actually matter since the failure would indicate that another thread has already updated the in memory packedRefList to either the same version this thread was trying to update it too, or to a more recent version. Either way, failing the CAS is then appropriate and should not be problematic. Although this change reverts the code in the RefDirectory class, it keeps the "improvements" to the test so that it continues to pass reliably. The reason for the quotes around the word "improvements" is because I believe the test alteration actually dramatically changes the intent of the test, and that the original intent of the test is untestable with the GC and RefDirectory classes as is. Bug: 582044 Change-Id: I3acee7527bb542996dcdfaddfb2bdb45ec444db5 Signed-off-by: Martin Fick <quic_mfick@quicinc.com> (cherry picked from commitc5617711a1
)
This commit is contained in:
parent
2fd050c567
commit
f6928f5736
|
@ -1032,22 +1032,7 @@ protected void writeFile(String name, byte[] content)
|
||||||
byte[] digest = Constants.newMessageDigest().digest(content);
|
byte[] digest = Constants.newMessageDigest().digest(content);
|
||||||
PackedRefList newPackedList = new PackedRefList(
|
PackedRefList newPackedList = new PackedRefList(
|
||||||
refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest));
|
refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest));
|
||||||
|
packedRefs.compareAndSet(oldPackedList, newPackedList);
|
||||||
// This thread holds the file lock, so no other thread or process should
|
|
||||||
// be able to modify the packed-refs file on disk. If the list changed,
|
|
||||||
// it means something is very wrong, so throw an exception.
|
|
||||||
//
|
|
||||||
// However, we can't use a naive compareAndSet to check whether the
|
|
||||||
// update was successful, because another thread might _read_ the
|
|
||||||
// packed refs file that was written out by this thread while holding
|
|
||||||
// the lock, and update the packedRefs reference to point to that. So
|
|
||||||
// compare the actual contents instead.
|
|
||||||
PackedRefList afterUpdate = packedRefs.updateAndGet(
|
|
||||||
p -> p.id.equals(oldPackedList.id) ? newPackedList : p);
|
|
||||||
if (!afterUpdate.id.equals(newPackedList.id)) {
|
|
||||||
throw new ObjectWritingException(
|
|
||||||
MessageFormat.format(JGitText.get().unableToWrite, name));
|
|
||||||
}
|
|
||||||
if (changed) {
|
if (changed) {
|
||||||
modCnt.incrementAndGet();
|
modCnt.incrementAndGet();
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue