From 53637790e8c0a7137437ea05aa8c92e1e5cd9484 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Fri, 13 Dec 2024 11:04:14 +0200 Subject: [PATCH] fix: circular dependency in qlist (#4302) * fix: circular dependency in qlist fixes #4294 Signed-off-by: Roman Gershman * chore: fixes Signed-off-by: Roman Gershman --------- Signed-off-by: Roman Gershman --- src/core/qlist.cc | 14 ++++++++++---- src/core/qlist_test.cc | 9 +++++++++ tests/dragonfly/seeder/script-genlib.lua | 4 +--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/core/qlist.cc b/src/core/qlist.cc index 0d676069b..f8cf6fede 100644 --- a/src/core/qlist.cc +++ b/src/core/qlist.cc @@ -371,6 +371,7 @@ string QList::Pop(Where where) { /* The head and tail should never be compressed */ DCHECK(node->encoding != QUICKLIST_NODE_ENCODING_LZF); + DCHECK(head_->prev->next == nullptr); string res; if (ABSL_PREDICT_FALSE(QL_NODE_IS_PLAIN(node))) { @@ -390,6 +391,7 @@ string QList::Pop(Where where) { } DelPackedIndex(node, pos); } + DCHECK(head_ == nullptr || head_->prev->next == nullptr); return res; } @@ -479,11 +481,13 @@ bool QList::PushSentinel(string_view value, Where where) { if (len_ == 1) { // sanity check DCHECK_EQ(malloc_size_, orig->sz); } + DCHECK(head_->prev->next == nullptr); return false; } quicklistNode* node = CreateFromSV(QUICKLIST_NODE_CONTAINER_PACKED, value); InsertNode(orig, node, opt); + DCHECK(head_->prev->next == nullptr); return true; } @@ -837,13 +841,15 @@ quicklistNode* QList::ListpackMerge(quicklistNode* a, quicklistNode* b) { void QList::DelNode(quicklistNode* node) { if (node->next) node->next->prev = node->prev; - if (node->prev) - node->prev->next = node->next; if (node == head_) { head_ = node->next; - } else if (node == head_->prev) { // tail - head_->prev = node->prev; + } else { + // for non-head nodes, update prev->next to point to node->next + // (If node==head, prev is tail and should always point to NULL). + node->prev->next = node->next; + if (node == head_->prev) // tail + head_->prev = node->prev; } /* Update len first, so in Compress we know exactly len */ diff --git a/src/core/qlist_test.cc b/src/core/qlist_test.cc index 9e5d67c07..64d1b5278 100644 --- a/src/core/qlist_test.cc +++ b/src/core/qlist_test.cc @@ -282,6 +282,15 @@ TEST_F(QListTest, CompressionPlain) { EXPECT_EQ(500, i); } +TEST_F(QListTest, LargeValues) { + string val(100000, 'a'); + ql_.Push(val, QList::HEAD); + ql_.Push(val, QList::HEAD); + ql_.Pop(QList::HEAD); + auto items = ToItems(); + EXPECT_THAT(items, ElementsAre(val)); +} + using FillCompress = tuple; class PrintToFillCompress { diff --git a/tests/dragonfly/seeder/script-genlib.lua b/tests/dragonfly/seeder/script-genlib.lua index 45d672a0a..2203df175 100644 --- a/tests/dragonfly/seeder/script-genlib.lua +++ b/tests/dragonfly/seeder/script-genlib.lua @@ -56,9 +56,7 @@ end function LG_funcs.add_list(key, keys) local is_huge = keys[key] - --- TODO -- investigate why second case of replication_test_all fails - --- we somehow create a quicklist that is circular and we deadlock - redis.apcall('LPUSH', key, unpack(randstr_sequence(false))) + redis.apcall('LPUSH', key, unpack(randstr_sequence(is_huge))) end function LG_funcs.mod_list(key, keys)