Skip to content

Commit 188a741

Browse files
lerman25lerman25
authored
[MOD-10681] Fix cursor list empty (#6594)
* fix cursorList_Empty to mark delete instead of instant delete * remove redundant GC call * remove unused destroy * remove Destroy from h file * update owernship test * fix comments * expand test to multiple cursors * fix loop * use std::vector and std::find
1 parent 9c3865c commit 188a741

3 files changed

Lines changed: 109 additions & 22 deletions

File tree

‎src/cursor.c‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -357,23 +357,22 @@ void Cursors_RenderStatsForInfo(CursorList *cl, CursorList *cl_coord, const Inde
357357
}
358358
#endif // FTINFO_FOR_INFO_MODULES
359359

360-
void CursorList_Destroy(CursorList *cl) {
361-
Cursors_GCInternal(cl, 1);
360+
void CursorList_Empty(CursorList *cl) {
361+
CursorList_Lock(cl);
362362
for (khiter_t ii = 0; ii != kh_end(cl->lookup); ++ii) {
363363
if (!kh_exist(cl->lookup, ii)) {
364364
continue;
365365
}
366-
Cursor *c = kh_val(cl->lookup, ii);
367-
Cursor_FreeInternal(c);
366+
Cursor *cur = kh_val(cl->lookup, ii);
367+
if (Cursor_IsIdle(cur)) {
368+
// Since the cursor is idle, we can free it.
369+
Cursor_RemoveFromIdle(cur);
370+
Cursor_FreeInternal(cur);
371+
} else {
372+
// Since the cursor is not idle, we mark it for deletion.
373+
// The next time the cursor is accessed, it will be freed.
374+
cur->delete_mark = true;
375+
}
368376
}
369-
kh_destroy(cursors, cl->lookup);
370-
371-
pthread_mutex_destroy(&cl->lock);
372-
Array_Free(&cl->idle);
373-
}
374-
375-
void CursorList_Empty(CursorList *cl) {
376-
bool is_coord = cl->is_coord;
377-
CursorList_Destroy(cl);
378-
CursorList_Init(cl, is_coord);
377+
CursorList_Unlock(cl);
379378
}

‎src/cursor.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -130,15 +130,11 @@ static inline CursorList *GetGlobalCursor(uint64_t cid) {
130130
*/
131131
void CursorList_Init(CursorList *cl, bool is_coord);
132132

133-
/**
134-
* Clear the cursor list
135-
*/
136-
void CursorList_Destroy(CursorList *cl);
137-
138133
/**
139134
* Empty the cursor list.
140-
* It is assumed that this function is called from the main thread, and that
141-
* are are no cursors that run in the background.
135+
* This function is thread-safe and handles both idle and active cursors.
136+
* Idle cursors are freed immediately, while active cursors are marked for
137+
* deletion and will be freed when they are next accessed.
142138
*/
143139
void CursorList_Empty(CursorList *cl);
144140

‎tests/cpptests/test_cpp_cursors.cpp‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,19 @@
1010

1111
#include "gtest/gtest.h"
1212
#include "cursor.h"
13+
#include <vector>
14+
#include <algorithm>
1315

1416
#define is_Idle(cur) ((cur)->pos != -1)
1517

1618
class CursorsTest : public ::testing::Test {};
1719

20+
bool IdInArray(uint64_t id, const uint64_t *arr, int size) {
21+
return std::find(arr, arr + size, id) != arr + size;
22+
}
23+
24+
25+
1826
TEST_F(CursorsTest, BasicAPI) {
1927
StrongRef dummy = {0};
2028
Cursor *cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
@@ -42,6 +50,8 @@ TEST_F(CursorsTest, BasicAPI) {
4250

4351
TEST_F(CursorsTest, OwnershipAPI) {
4452
StrongRef dummy = {0};
53+
54+
// Case 1: Cursors_Purge marks non-idle cursor for deletion
4555
Cursor *cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
4656
ASSERT_TRUE(cur != NULL);
4757
ASSERT_FALSE(cur->delete_mark);
@@ -56,7 +66,7 @@ TEST_F(CursorsTest, OwnershipAPI) {
5666
ASSERT_EQ(Cursor_Pause(cur), REDISMODULE_OK) << "Pausing the cursor Should actually free it.";
5767
ASSERT_EQ(Cursors_GetInfoStats().total_user, 0) << "Cursor should be deleted";
5868

59-
// Try again with explicitly deleting the cursor
69+
// Case 2: Cursors_Purge with explicit cursor free
6070
cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
6171
ASSERT_TRUE(cur != NULL);
6272
ASSERT_FALSE(cur->delete_mark);
@@ -72,4 +82,86 @@ TEST_F(CursorsTest, OwnershipAPI) {
7282
ASSERT_EQ(Cursor_Free(cur), REDISMODULE_OK) << "Cursor should be deleted";
7383
ASSERT_EQ(Cursors_GetInfoStats().total_user, 0) << "Cursor should be deleted";
7484

85+
// Case 3: CursorList_Empty marks non-idle cursor for deletion
86+
cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
87+
ASSERT_TRUE(cur != NULL);
88+
ASSERT_FALSE(cur->delete_mark);
89+
ASSERT_FALSE(is_Idle(cur));
90+
id = cur->id;
91+
92+
// Call CursorList_Empty while cursor is not idle (active)
93+
CursorList_Empty(&g_CursorsList);
94+
95+
// Cursor should be marked for deletion, not immediately freed
96+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 1) << "Cursor should still be alive";
97+
ASSERT_EQ(Cursors_TakeForExecution(&g_CursorsList, id), nullptr) << "Cursor already deleted";
98+
ASSERT_TRUE(cur->delete_mark) << "Cursor should be marked for deletion";
99+
100+
// When cursor is paused, it should actually be freed due to delete_mark
101+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 1) << "Cursor should be alive";
102+
ASSERT_EQ(Cursor_Pause(cur), REDISMODULE_OK) << "Pausing the cursor should actually free it";
103+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 0) << "Cursor should be deleted";
104+
105+
// Case 4: CursorList_Empty with explicit cursor free
106+
cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
107+
ASSERT_TRUE(cur != NULL);
108+
ASSERT_FALSE(cur->delete_mark);
109+
ASSERT_FALSE(is_Idle(cur));
110+
id = cur->id;
111+
112+
// Call CursorList_Empty while cursor is not idle (active)
113+
CursorList_Empty(&g_CursorsList);
114+
115+
// Cursor should be marked for deletion, not immediately freed
116+
ASSERT_TRUE(cur->delete_mark) << "Cursor should be marked for deletion";
117+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 1) << "Cursor should still be alive";
118+
119+
// When cursor is explicitly freed, it should be deleted
120+
ASSERT_EQ(Cursor_Free(cur), REDISMODULE_OK) << "Cursor should be deleted";
121+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 0) << "Cursor should be deleted";
122+
123+
// Case 5: CursorList_Empty on multiple cursors, some idle, some active
124+
// Verify that the idle cursors are freed immediately, and the active ones are marked for deletion
125+
constexpr int numCursors = 5;
126+
constexpr int numIdle = numCursors / 2 + numCursors % 2;
127+
std::vector<uint64_t> ids;
128+
129+
for (int i = 0; i < numCursors; ++i) {
130+
cur = Cursors_Reserve(&g_CursorsList, dummy, 1000, NULL);
131+
ASSERT_TRUE(cur != NULL);
132+
ASSERT_FALSE(cur->delete_mark);
133+
ASSERT_FALSE(is_Idle(cur));
134+
if (i % 2 == 0) {
135+
ASSERT_EQ(Cursor_Pause(cur), REDISMODULE_OK) << "Cursor should be paused";
136+
ids.push_back(cur->id);
137+
}
138+
}
139+
140+
141+
142+
ASSERT_EQ(Cursors_GetInfoStats().total_user, numCursors) << "All cursors should be alive";
143+
144+
// Call CursorList_Empty
145+
CursorList_Empty(&g_CursorsList);
146+
147+
// The Idle cursors should be freed immediately, the active ones should be marked for deletion
148+
ASSERT_EQ(Cursors_GetInfoStats().total_user, numCursors - numIdle) << "Half of the cursors should be alive";
149+
150+
// Verify the Ids of the cursors alive
151+
for (khiter_t ii = 0; ii != kh_end(g_CursorsList.lookup); ++ii) {
152+
if (!kh_exist(g_CursorsList.lookup, ii)) {
153+
continue;
154+
}
155+
Cursor *cur = kh_val(g_CursorsList.lookup, ii);
156+
// Assert mark delete
157+
158+
ASSERT_TRUE(cur->delete_mark) << "Cursor should be marked for deletion";
159+
ASSERT_FALSE(IdInArray(cur->id, ids.data(), ids.size())) << "Cursor should not be in the deleted array";
160+
// Pause the cursor
161+
ASSERT_EQ(Cursor_Pause(cur), REDISMODULE_OK) << "Cursor should be paused";
162+
}
163+
164+
// After the cursors are paused, they should be freed
165+
ASSERT_EQ(Cursors_GetInfoStats().total_user, 0) << "All cursors should be deleted";
166+
75167
}

0 commit comments

Comments
 (0)