diff --git a/SkipList.cpp b/SkipList.cpp index 5dee515..d64d550 100644 --- a/SkipList.cpp +++ b/SkipList.cpp @@ -819,10 +819,17 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl { } void setOldestVersion(int64_t oldestVersion) { + // oldestVersion becomes the max of its previous value and the argument, so + // an older version is a no-op. We still fall through in the equal case so + // that garbage collection can make progress without advancing the oldest + // version. + if (oldestVersion < this->oldestVersion) { + return; + } + // This isn't 100% accurate. It overcounts if you hit the end gc_iterations_total.add(keyUpdates); - assert(oldestVersion >= this->oldestVersion); this->oldestVersion = oldestVersion; SkipList::Finger finger; int temp; diff --git a/test_conflict_set.py b/test_conflict_set.py index 67a5780..bc30a9a 100644 --- a/test_conflict_set.py +++ b/test_conflict_set.py @@ -68,6 +68,35 @@ def test_hash_table_getBytes(): assert cs.check(read(0, b"key")) == [Result.CONFLICT] +def test_setOldestVersion_does_not_decrease(): + # Regression test for issue #87: `setOldestVersion` sets oldestVersion to + # the maximum of its previous value and its argument, so passing an older + # version is valid input and must be a no-op. skip_list used to assert in + # Debug builds and, in Release builds, lower its oldest version, which + # resurrected a window of versions that callers were told resulted in + # TooOld. + with DebugConflictSet() as cs: + cs.addWrites(5, write(b"k")) + cs.setOldestVersion(5) + # Valid: `2 >= 0` and `2 <= newestVersion == 5`. oldestVersion stays 5, + # so 3 and 4 are still TooOld. + cs.setOldestVersion(2) + assert cs.check(read(3, b"k")) == [Result.TOO_OLD] + assert cs.check(read(4, b"k")) == [Result.TOO_OLD] + assert cs.check(read(5, b"k")) == [Result.COMMIT] + + # Same, but with the entries at the old oldestVersion already garbage + # collected, so lowering oldestVersion would report Commit for a version + # that the caller was told resulted in TooOld. + with DebugConflictSet() as cs: + cs.addWrites(1, write(b"a")) + cs.addWrites(2) # No writes; advances newestVersion to 2 + cs.setOldestVersion(2) # Garbage collects the write at version 1 + cs.setOldestVersion(1) + assert cs.check(read(1, b"a")) == [Result.TOO_OLD] + assert cs.check(read(2, b"a")) == [Result.COMMIT] + + def test_hash_table_setOldestVersion_empties_map(): # Regression test for issue #78: setOldestVersion must terminate when # its garbage-collection pass erases every entry in the map. Each write