diff --git a/CMakeLists.txt b/CMakeLists.txt index 733818c..f5f1e0c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -182,6 +182,8 @@ endif() include(CTest) +option(TEST_SKIP_LIST "Run the slow skip list cross-check tests" OFF) + # disable tests if this is being used through e.g. FetchContent if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING) @@ -224,8 +226,9 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING) target_compile_options(driver_skip_list PRIVATE ${TEST_FLAGS}) target_link_libraries(driver_skip_list PRIVATE skip_list) - # enable to test skip list - if(0) + # Optional cross-check of SkipList.cpp against ReferenceImpl. Opt-in via + # -DTEST_SKIP_LIST=ON, since these are slow. + if(TEST_SKIP_LIST) foreach(TEST ${CORPUS_TESTS}) get_filename_component(hash ${TEST} NAME) add_test(NAME skip_list_${hash} COMMAND driver_skip_list ${TEST}) 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