Skip to content

Isolate blocking Redis reader connections - #127

Merged
derekste merged 2 commits into
mainfrom
dev/reader-connection-policy
Oct 7, 2026
Merged

derekste merged 2 commits into
mainfrom
dev/reader-connection-policy

Conversation

@derekste

Copy link
Copy Markdown
Member

Blocking XREADs currently consume the command pool, so increasing reader count can stall unrelated writes and health checks. Give blocking reads a separate pool sized to RA_Options::readers, keep connection establishment independent of the command socket deadline, and preserve established clients when a replacement connection fails.

Physical reads finish within one second, including when command timeout is zero, with socket-deadline slack for normal idle replies. The new regressions cover idle reads, failed replacement, eight blocked readers with a one-connection command pool, and finite zero-timeout cycles. The existing tests now accept a private TCP/Unix-socket fixture and use atomic callback wait flags. Stacked PRs run the normal CI job and caller-selected C++20 is preserved.

Split from the connection-policy findings in #108 for focused review. @bigsamich, the subscription and recovery PRs will be stacked above this foundation.

Validation: native Release builds on macOS and Linux; Linux uses the pinned Redis 7.4.2 image and confirmed -std=gnu++20. All 18 Linux CTest cases passed, and all four focused connection-policy cases passed on macOS.

@bigsamich bigsamich left a comment

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.

Separating the reader pool from the command pool makes sense. A few issues I'd like addressed before merge:

Blocking

  1. Reader dies on a slow reply (RedisConnection.hpp ~320). Removing the swr::TimeoutError catch makes xreadMultiBlock return false on a socket timeout. The reader loop (RedisAdapter.cpp ~404) then sets info.run = false and the thread exits for good. With timeout=500 the reader deadline is 1250 ms, so a stall of about 750 ms ends the reader. Writes and pings still succeed, so reconnect() never runs and the reader stays dead. Keep the catch, or retry instead of exiting.
  2. timeout == 0 can drop entries (~314). It used to block indefinitely. It's now capped at 1 s, and until the first entry arrives the IDs are still $. An XADD that lands between two cycles is never delivered.
  3. Cluster: reader pool undersized (~135). The pool holds readers connections per node (default 1), but there's one reader thread per slot × shard. Those threads queue on a pool with wait_timeout = 0 (no limit), which delays data. stop_reader() also waits in that queue while holding _reader_mtx.

Behavior changes
4. A failed connect() (~142) now keeps the old clients instead of clearing them. During an outage each command blocks for the connect timeout instead of returning RA_NOT_CONNECTED right away.
5. connectTimeout defaults to 500 ms (~98), where redis-plus-plus defaults to no limit. Slow or tunneled links that connect today may stop connecting.

Tests / CI
6. connection_policy_test.cpp:47 builds a std::string from getenv("REDIS_ADAPTER_TEST_SOCKET") without a null check, so the test crashes when that variable is unset.
7. connection_policy_test.cpp:92 counts every cmd=xread client on the server and expects exactly 8. It flakes under ctest -j (MultiReader adds 16) and against the shared CI Redis.
8. test-and-benchmark.yml doesn't set the new env vars, so the policy tests skip there but still report as passing.

Nits
9. The 1000 ms cycle cap is written at both ~133 and ~314. A shared constant would keep the socket deadline above the block time.
10. The Options doc comment still says timeout covers connection. It doesn't mention connectTimeout or readerPoolSize.

@derekste

derekste commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Both reported failures reproduce on PR #127 at 310babb: delaying an XREAD reply beyond its socket deadline permanently stops the reader while command PING remains healthy, and publishing between finite timeout=0 cycles loses the entry while the cursor remains '$'.

The lifecycle implementation in PR #108 at b6c442a already addresses both causes. It retries failed reads with bounded backoff and resolves '$' to each stream's concrete tail before polling, keeping unresolved keys out of repeated XREAD calls. The same controlled scenarios pass on exact #108, #109 (7256f99), #111 (94470f5), and final #128 (5162997): each delivers the expected entry once and excludes pre-registration history. These results come from isolated standalone Redis 7.0.12 tests; they do not claim full-suite or Cluster coverage.

I propose treating the two findings as superseded in the final stack, with a required landing condition: #127 must not ship alone. The owner must accept the ordered foundation-plus-#108 landing without any foundation-only intermediate deployment, or request regrouping before submission. No duplicate production change is proposed.

@derekste
derekste requested a review from wsulli October 7, 2026 15:36
@derekste
derekste merged commit 6883068 into main Oct 7, 2026
1 check passed
@derekste
derekste deleted the dev/reader-connection-policy branch October 7, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants