Add client blocking mechanism for keys in use (forkless-pre-bgiterator) by harrylin98 · Pull Request #3349 · valkey-io/valkey

harrylin98 · 2026-03-10T16:31:26Z

This is pre-submission into the forkless-pre-bgiterator branch for client blocking mechanism. The actual PR review for this is here: #3341 Submitting to this branch to enable PR review for bgIterator.

A client blocking system (blockedInUse) that prevents concurrent access to keys actively being modified by internal operations (e.g., bgIteration). The mechanism blocks clients attempting to access in-use keys and automatically unblocks them when keys become available. Signed-off-by: harrylin98 <harrylin980107@gmail.com>

Copilot

Pull request overview

Introduces a new blocked_inuse subsystem to block/unblock clients attempting to access keys currently “in use” by internal operations, and wires it into the core client lifecycle (unblocking, timeouts, client unlink/free) along with unit tests.

Changes:

Add blocked_inuse.{c,h} implementing key-based client blocking with mappings between clients and keys.
Integrate the new blocking state into client timeout handling, processUnblockedClients(), unlinkClient(), and client info flags.
Add unit tests and build-system registration (Makefile + CMake) for the new module.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/blocked_inuse.c	Implements the new blocking mechanism and data structures.
src/blocked_inuse.h	Public header for the blocking API.
src/blocked.c	Restores read handlers for unblocked clients and adds pause interaction/assertions.
src/timeout.c	Prevents idle timeout for `blockInuse`-blocked clients.
src/networking.c	Ensures blocked-inuse clients are unlinked safely; updates CLIENT LIST flags; adjusts pending processing for closing clients.
src/server.h	Adds a new client flag bit for `blockInuse` state.
src/server.c	Initializes/releases `blockInuse`; adds TCP-close detection logic in clients cron; asserts blocked state isn’t processed as a command.
src/unit/test_blockedInuse.cpp	Adds gtest coverage for the new blocking behavior.
src/unit/wrappers.h	Adds wrappers needed to mock networking/db functions in unit tests.
src/Makefile	Adds `blocked_inuse.o` to server objects.
cmake/Modules/SourceFiles.cmake	Adds `blocked_inuse.c` to CMake source list.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T08:58:41Z

+    for (int i = 0; i < nKeys; ++i) {
+        serverAssert(keys[i]->type == OBJ_STRING);
+        // Verify key exists in at least one database across the server
+        int found = 0;
+        for (int j = 0; j < server.dbnum; ++j) {
+            if (lookupKeyRead(server.db[j], keys[i]) != NULL) {
+                found = 1;
+                break;
+            }
+        }
+        serverAssert(found);
+    }


The existence check calls lookupKeyRead(server.db[j], keys[i]), but lookupKeyRead() uses objectGetVal(key) as the key name. Here keys[i] appears to be the DB value object (with embedded key via objectSetKeyAndExpire), so objectGetVal(keys[i]) is the value payload, not the key name—this makes the check incorrect and can serverAssert(found) for valid keys. Either remove this validation or look up using the embedded key name (objectGetKey(keys[i])) and verify the returned value matches the expected object.

Copilot · 2026-03-11T08:58:42Z

+#if defined(__linux__)
+    // Check TCP socket state using Linux TCP_INFO
+    struct tcp_info info;
+    socklen_t infolen = sizeof(info);
+    if (getsockopt(c->conn->fd, IPPROTO_TCP, TCP_INFO, &info, &infolen) != 0 || infolen < sizeof(info)) return false; // Cannot retrieve TCP info
+    bool connection_is_closing = (info.tcpi_state == TCP_CLOSE_WAIT || info.tcpi_state == TCP_CLOSE);
+#elif defined(__APPLE__)
+    // Check TCP socket state using macOS TCP_CONNECTION_INFO
+    struct tcp_connection_info info;
+    socklen_t infolen = sizeof(info);
+    if (getsockopt(c->conn->fd, IPPROTO_TCP, TCP_CONNECTION_INFO, &info, &infolen) != 0 || infolen < sizeof(info)) return false; // Cannot retrieve TCP info
+    bool connection_is_closing = (info.tcpi_state == TCPS_CLOSE_WAIT || info.tcpi_state == TCPS_CLOSED);
+#endif
+
+    if (connection_is_closing) {
+        if (server.verbosity <= LL_VERBOSE) {


clientsCronTcpIsClosing() only defines connection_is_closing inside the linux/APPLE branches. On other supported platforms this will not compile (connection_is_closing is undefined). Add a fallback (#else) that returns false (or implement the platform-specific check) and guard platform headers accordingly.

Copilot · 2026-03-11T08:58:42Z

    bio.o \
    bitops.o \
    blocked.o \
+	blocked_inuse.o \


The new object list entry uses a leading tab while the surrounding ENGINE_SERVER_OBJ entries use spaces. This can make the Makefile harder to read and can cause whitespace churn in future edits. Align indentation with the surrounding lines (spaces).

Suggested change

blocked_inuse.o \

blocked_inuse.o \

Copilot · 2026-03-11T08:58:42Z

+ConnectionType BlockedInuseTest::dummyConnType = {0};
+
+
+TEST_F(BlockedInuseTest, blockInitialState) {


This test case is empty and doesn't assert any behavior, so it won't catch regressions and adds noise to the test suite. Either add assertions for the initial blockInuse state or remove the test.

Suggested change

TEST_F(BlockedInuseTest, blockInitialState) {

TEST_F(BlockedInuseTest, blockInitialState) {

/* Verify that the blockInuse state is clean at the beginning of the test. */

EXPECT_EQ(blockInuse_getNumberOfBlockedClients(), 0);

EXPECT_EQ(blockInuse_getNumberOfBlockedKeys(), 0);

EXPECT_EQ(listLength(server.unblocked_clients), 0UL);

/* Verify that a newly created client is not blocked or unblocked yet. */

client *c = createFakeClient(1);

verifyClientBlockState(c, 0, 0);

EXPECT_NE(c->conn->read_handler, nullptr);

freeFakeClient(c);

Copilot · 2026-03-11T08:58:42Z

+static hashtableType keyToClientsHashtableType = {
+    .entryGetKey = keyToClientsGetKey,
+    .hashFunction = dictEncObjHash,
+    .keyCompare = hashtableEncObjKeyCompare,


key_to_clients is using dictEncObjHash/hashtableEncObjKeyCompare, which hash/compare based on objectGetVal(). For the value objects stored in the DB, objectGetVal() is the value (mutable and not unique per key), so different keys with the same value will collide and updates to the value will break hashtable lookups. This will cause clients to be blocked/unblocked on the wrong keys or never unblocked. Key the table by the key name (objectGetKey()) or an sds key string (using sds hash/compare), rather than hashing the value payload.

Suggested change

static hashtableType keyToClientsHashtableType = {

.entryGetKey = keyToClientsGetKey,

.hashFunction = dictEncObjHash,

.keyCompare = hashtableEncObjKeyCompare,

/*

* Hash and compare functions for key_to_clients:

* We must key the hashtable by the key *name*, not by the mutable value

* object stored in the DB. We therefore derive the key-name object via

* objectGetKey() and delegate hashing/comparison to the generic helpers.

*/

static uint64_t keyToClientsHashByKeyName(const void *key) {

robj *o = (robj *)key;

robj *keyname = objectGetKey(o);

serverAssert(keyname != NULL);

return dictEncObjHash(keyname);

}

static int keyToClientsKeyCompareByKeyName(const void *key1, const void *key2) {

robj *o1 = (robj *)key1;

robj *o2 = (robj *)key2;

robj *k1 = objectGetKey(o1);

robj *k2 = objectGetKey(o2);

serverAssert(k1 != NULL && k2 != NULL);

return hashtableEncObjKeyCompare(k1, k2);

}

static hashtableType keyToClientsHashtableType = {

.entryGetKey = keyToClientsGetKey,

.hashFunction = keyToClientsHashByKeyName,

.keyCompare = keyToClientsKeyCompareByKeyName,

…r) (valkey-io#3349) This is pre-submission into the `forkless-pre-bgiterator` branch for client blocking mechanism. The actual PR review for this is here: valkey-io#3341 Submitting to this branch to enable PR review for bgIterator. Signed-off-by: harrylin98 <harrylin980107@gmail.com>

github-actions Bot assigned harrylin98 Mar 10, 2026

JimB123 changed the title ~~Add client blocking mechanism for keys in use~~ Add client blocking mechanism for keys in use (forkless-pre-bgiterator) Mar 10, 2026

harrylin98 requested a review from JimB123 March 10, 2026 17:23

madolson requested a review from Copilot March 11, 2026 08:42

Copilot started reviewing on behalf of madolson March 11, 2026 08:46 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

JimB123 merged commit c039e4e into valkey-io:forkless-pre-bgiterator Mar 11, 2026
57 checks passed

Navigation Menu

Uh oh!

Add client blocking mechanism for keys in use (forkless-pre-bgiterator) - #3349

Add client blocking mechanism for keys in use (forkless-pre-bgiterator)#3349
JimB123 merged 1 commit into
valkey-io:forkless-pre-bgiteratorfrom
harrylin98:blockedInUse-for-pr

harrylin98 commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		ConnectionType BlockedInuseTest::dummyConnType = {0};


		TEST_F(BlockedInuseTest, blockInitialState) {

-TEST_F(BlockedInuseTest, blockInitialState) {
+TEST_F(BlockedInuseTest, blockInitialState) {
+    /* Verify that the blockInuse state is clean at the beginning of the test. */
+    EXPECT_EQ(blockInuse_getNumberOfBlockedClients(), 0);
+    EXPECT_EQ(blockInuse_getNumberOfBlockedKeys(), 0);
+    EXPECT_EQ(listLength(server.unblocked_clients), 0UL);
+    /* Verify that a newly created client is not blocked or unblocked yet. */
+    client *c = createFakeClient(1);
+    verifyClientBlockState(c, 0, 0);
+    EXPECT_NE(c->conn->read_handler, nullptr);
+    freeFakeClient(c);

-static hashtableType keyToClientsHashtableType = {
-    .entryGetKey = keyToClientsGetKey,
-    .hashFunction = dictEncObjHash,
-    .keyCompare = hashtableEncObjKeyCompare,
+/*
+ * Hash and compare functions for key_to_clients:
+ * We must key the hashtable by the key *name*, not by the mutable value
+ * object stored in the DB. We therefore derive the key-name object via
+ * objectGetKey() and delegate hashing/comparison to the generic helpers.
+ */
+static uint64_t keyToClientsHashByKeyName(const void *key) {
+    robj *o = (robj *)key;
+    robj *keyname = objectGetKey(o);
+    serverAssert(keyname != NULL);
+    return dictEncObjHash(keyname);
+}
+static int keyToClientsKeyCompareByKeyName(const void *key1, const void *key2) {
+    robj *o1 = (robj *)key1;
+    robj *o2 = (robj *)key2;
+    robj *k1 = objectGetKey(o1);
+    robj *k2 = objectGetKey(o2);
+    serverAssert(k1 != NULL && k2 != NULL);
+    return hashtableEncObjKeyCompare(k1, k2);
+}
+static hashtableType keyToClientsHashtableType = {
+    .entryGetKey = keyToClientsGetKey,
+    .hashFunction = keyToClientsHashByKeyName,
+    .keyCompare = keyToClientsKeyCompareByKeyName,

Uh oh!

Conversation

harrylin98 commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants