Skip to content

Avoid holding the Sentinel mutex across I/O - #319

Closed
OskarEichler wants to merge 1 commit into
redis-rb:masterfrom
OskarEichler:codex/sentinel-fiber-concurrency
Closed

Avoid holding the Sentinel mutex across I/O#319
OskarEichler wants to merge 1 commit into
redis-rb:masterfrom
OskarEichler:codex/sentinel-fiber-concurrency

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Move Sentinel network I/O outside the shared mutex, single-flight resolution across threads, allow same-thread fibers to progress with independent short-lived clients, and make reset and Sentinel-list updates concurrency-safe.

Verification: the same-thread fiber reproduction changes ThreadError to two successful resolutions; 20 concurrent threads perform one resolution; focused Ruby 4.0.6 checks, RuboCop, and gem build pass. The full Sentinel integration harness was unreliable locally on Ruby 4 / Redis 7, so upstream CI is requested. Source-only patch; no tests included.

@byroot byroot closed this Aug 30, 2026
@OskarEichler

OskarEichler commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

@byroot Thanks for reviewing this. Could you clarify why the PR was closed—was the concurrency model unsuitable, or is there another intended direction for Sentinel resolution?

The reproduced issue is that Sentinel network I/O occurs while holding the shared mutex: same-thread fibers can raise ThreadError, and one slow Sentinel request serializes otherwise independent callers. The patch moves resolution outside the shared lock, single-flights competing threads, gives same-thread fibers independent short-lived clients, and synchronizes reset/Sentinel-list updates. In focused verification, the fiber reproduction changed from ThreadError to two successful resolutions, and 20 concurrent threads performed one resolution.

The full Sentinel integration harness was unreliable locally, so if closure was due to verification or scope I’m happy to revise the approach and add focused upstream regression tests.

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.

2 participants