Skip to content

Client blocking mechanism for keys in use (forkless)#3469

Merged
JimB123 merged 1 commit intovalkey-io:forklessfrom
harrylin98:blockInUse-unify
Apr 21, 2026
Merged

Client blocking mechanism for keys in use (forkless)#3469
JimB123 merged 1 commit intovalkey-io:forklessfrom
harrylin98:blockInUse-unify

Conversation

@harrylin98
Copy link
Copy Markdown
Contributor

@harrylin98 harrylin98 commented Apr 9, 2026

Summary

This is a building block for upcoming bgIteration that require exclusive key access without stalling the event loop.

This PR adds blockInUse, a server-initiated client blocking mechanism that prevents concurrent access to keys held exclusively by internal operations. When a client command targets a key that is currently in use, the client is blocked and its read handler is removed to prevent unbounded input buffering. Once the internal operation releases the key, the client is automatically resumed and its pending command is re-executed.

When a client is blocked by blockInUse, its read handler is removed so the event loop stops monitoring read events for that connection, preventing new commands from being buffered into c->querybuf while the client is waiting. Clients would otherwise continue buffering incoming commands during the entire blocking period, leading to unbounded memory growth across all blocked clients. The read handler is restored in processUnblockedClients() when the client is unblocked.

The tradeoff is that removing the read handler also makes the event loop blind to TCP disconnects on that client connection. To avoid leaking zombie file descriptors, clientsCronTcpIsClosing() is added to detect and free connections that were closed by the remote side while the read handler was removed.

Design decisions

Timeout

BLOCKED_INUSE clients do not time out. The timeout is explicitly set to zero before blocking, so clients remain blocked until the key is released by the internal operation.

Module commands without ALLOW_BLOCK

Module commands registered without ALLOW_BLOCK that target a key held by an internal operation will receive a INUSE key is being processed error. This is a new failure mode.

processUnblockedClients refactor

Replaced the silent skip of blocked non-module clients with an assert, since a non-module client being both
blocked and in the unblocked list is a bug. Added read handler restoration unblocked clients; for other blocking types
this is a no-op.

Testing

Unit tests (src/unit/test_blocked.cpp) cover each public API.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 97.17949% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.60%. Comparing base (3f88d60) to head (2fa56b3).
⚠️ Report is 1 commits behind head on forkless.

Files with missing lines Patch % Lines
src/module.c 0.00% 6 Missing ⚠️
src/blocked.c 95.14% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           forkless    #3469      +/-   ##
============================================
+ Coverage     76.56%   76.60%   +0.03%     
============================================
  Files           158      159       +1     
  Lines         79078    79464     +386     
============================================
+ Hits          60548    60870     +322     
- Misses        18530    18594      +64     
Files with missing lines Coverage Δ
src/connection.h 88.46% <100.00%> (+0.34%) ⬆️
src/networking.c 92.19% <100.00%> (-0.31%) ⬇️
src/rdma.c 100.00% <ø> (ø)
src/server.c 89.51% <100.00%> (+<0.01%) ⬆️
src/server.h 100.00% <ø> (ø)
src/socket.c 96.08% <100.00%> (+0.13%) ⬆️
src/tls.c 17.64% <ø> (ø)
src/unit/test_blocked.cpp 100.00% <100.00%> (ø)
src/unix.c 78.31% <ø> (ø)
src/blocked.c 92.03% <95.14%> (+0.90%) ⬆️
... and 1 more

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 3 times, most recently from c37061b to de6f63c Compare April 10, 2026 06:26
@harrylin98 harrylin98 marked this pull request as ready for review April 10, 2026 06:31
@harrylin98 harrylin98 added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Apr 10, 2026
@madolson madolson requested a review from Copilot April 10, 2026 18:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new server-initiated blocking mode (BLOCKED_INUSE) intended to temporarily block clients from accessing keys held exclusively by internal/background operations, while preventing unbounded query buffer growth by removing the client read handler and later restoring it when unblocked.

Changes:

  • Add BLOCKED_INUSE and a block/unblock API (blockClientInUseOnKeys, unblockClientsInUseOnKey, etc.) backed by a key→clients mapping.
  • Remove client read handlers while blocked-in-use, restore them in processUnblockedClients(), and add cron-based detection of remotely-closed connections while handlers are removed.
  • Extend ConnectionType with an is_closing hook, implement it for socket/TLS, and add unit tests for the new API.

Reviewed changes

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

Show a summary per file
File Description
src/blocked.c Implements BLOCKED_INUSE mechanism, key↔client tracking, and read-handler restore on unblock; adds guardrails in generic unblock paths.
src/server.h Adds BLOCKED_INUSE enum value and exports block-in-use public APIs.
src/networking.c Integrates unlink cleanup for blocked-in-use clients; adjusts free/unlink ordering; ensures blocked-in-use clients aren’t unblocked via the generic path.
src/server.c Initializes/releases block-in-use state; adds clientsCronTcpIsClosing() to reap connections closed while read handlers are removed.
src/connection.h Adds ConnectionType.is_closing hook.
src/socket.c Implements connSocketIsClosing() using TCP state via getsockopt (Linux/macOS) and wires it into socket connections.
src/tls.c Wires TLS ConnectionType.is_closing to the socket implementation.
src/unix.c Extends Unix connection type initializer with the new is_closing field (currently NULL).
src/rdma.c Extends RDMA connection type initializer with the new is_closing field (currently NULL).
src/unit/wrappers.h Adds wrappers for processPendingCommandAndInputBuffer and beforeNextClient to support mocking in tests.
src/unit/test_blockedInuse.cpp Adds unit tests covering block-in-use API behavior.

Comment thread src/blocked.c Outdated
Comment thread src/socket.c
Comment thread src/unix.c
Comment thread src/rdma.c
Comment thread src/unit/test_blocked.cpp
Comment thread src/unit/test_blocked.cpp
Comment thread src/blocked.c Outdated
Comment thread src/unit/test_blocked.cpp
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/networking.c Outdated
Comment thread src/networking.c Outdated
Comment thread src/blocked.c Outdated
@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 2 times, most recently from 07fea81 to 3ac8035 Compare April 13, 2026 20:33
Copy link
Copy Markdown
Contributor

@murphyjacob4 murphyjacob4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm partial - but I find the new code much simpler! Thanks 🥇

Comment thread src/blocked.c Outdated
Comment on lines +813 to +815
* Note: BLOCKED_INUSE never goes through unblockClient(). Blocking state is
* cleared via resetBlockInUseClientState() instead, and calling
* unblockClient() on a BLOCKED_INUSE client will trigger a serverPanic.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the comment?

Comment thread src/blocked.c Outdated
Comment thread src/server.c Outdated
Comment thread src/unit/test_blocked.cpp
@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 4 times, most recently from fee1370 to e27c633 Compare April 14, 2026 06:12
@harrylin98 harrylin98 requested a review from JimB123 April 14, 2026 16:36
@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 4 times, most recently from 89e0317 to e6896f4 Compare April 15, 2026 23:56
Comment thread src/blocked.c
Comment thread src/module.c Outdated
Comment thread src/connection.h
Comment thread src/networking.c Outdated
Comment thread src/server.h Outdated
Comment thread src/unit/test_blocked.cpp Outdated
@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 2 times, most recently from df759ff to 77fea02 Compare April 20, 2026 22:05
Copy link
Copy Markdown
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core mechanism is sound. Just some minor comments.

Comment thread src/blocked.c Outdated
Comment thread src/blocked.c
Comment thread src/blocked.c
Comment thread src/server.c Outdated
Comment thread src/tls.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/module.c
Comment thread src/socket.c
Comment thread src/networking.c
Comment thread src/server.c Outdated
@harrylin98 harrylin98 force-pushed the blockInUse-unify branch 2 times, most recently from 70c0d9f to 31303fd Compare April 21, 2026 03:19
@JimB123 JimB123 changed the title Add client blocking mechanism for keys in use Add client blocking mechanism for keys in use (forkless) Apr 21, 2026
@JimB123 JimB123 changed the title Add client blocking mechanism for keys in use (forkless) Client blocking mechanism for keys in use (forkless) Apr 21, 2026
A client blocking system (blockInUse) 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.

When a client is blocked by blockInUse, its read handler is removed so the event
loop stops monitoring read events for that connection, preventing new commands
from being buffered into c->querybuf while the client is waiting. The read
handler is restored in processUnblockedClients() when the client is unblocked.

To avoid leaking zombie file descriptors, clientsCronTcpIsClosing() is added to
detect and free connections that were closed by the remote side while the read
handler was removed.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
@JimB123 JimB123 merged commit 6c19040 into valkey-io:forkless Apr 21, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants