[Issue #35] Add mutex protection to ConsistentHashRing for thread safety #40

HugoNijhuis · 2026-01-10T14:31:09Z

HugoNijhuis commented

2026-01-10 14:31:09 +00:00

Closes #35

Summary

Added sync.RWMutex to ConsistentHashRing struct for thread-safe concurrent access
Write operations (AddNode, RemoveNode) use exclusive locks (Lock/Unlock)
Read operations (GetNode, GetNodes, IsEmpty) use shared locks (RLock/RUnlock)

Impact

This prevents race conditions when multiple goroutines access the hash ring concurrently, which is critical for cluster membership changes happening alongside key lookups.

Test Plan

All existing tests pass
Tests pass with -race flag enabled (race detector)

Closes #35 ## Summary - Added `sync.RWMutex` to `ConsistentHashRing` struct for thread-safe concurrent access - Write operations (`AddNode`, `RemoveNode`) use exclusive locks (`Lock`/`Unlock`) - Read operations (`GetNode`, `GetNodes`, `IsEmpty`) use shared locks (`RLock`/`RUnlock`) ## Impact This prevents race conditions when multiple goroutines access the hash ring concurrently, which is critical for cluster membership changes happening alongside key lookups. ## Test Plan - [x] All existing tests pass - [x] Tests pass with `-race` flag enabled (race detector)

HugoNijhuis added 1 commit 2026-01-10 14:31:10 +00:00

Add mutex protection to ConsistentHashRing for thread safety

CI / build (pull_request) Successful in 17s

Details

d33477c02a

- Add sync.RWMutex to ConsistentHashRing struct
- Use Lock/Unlock for write operations (AddNode, RemoveNode)
- Use RLock/RUnlock for read operations (GetNode, GetNodes, IsEmpty)

This allows concurrent reads (the common case) while serializing writes,
preventing race conditions when multiple goroutines access the hash ring.

Closes #35

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

HugoNijhuis commented

2026-01-10 14:37:57 +00:00

Code Review for PR #40

Summary

This PR adds mutex protection to ConsistentHashRing for thread safety, addressing Issue #35.

Analysis

Positive aspects:

Correct lock granularity: Uses RWMutex with RLock for read operations (GetNode, GetNodes, IsEmpty) and Lock for write operations (AddNode, RemoveNode).
Complete coverage: All public methods that access shared state are protected. The private hash method correctly does not have its own lock since it is only called from within already-locked sections.
Consistent use of defer: All lock/unlock pairs use defer, ensuring locks are released even if early returns or panics occur.
No deadlock risk: The implementation does not call any external code while holding locks.
Tests pass with race detector: go test -race ./cluster/... passes.

Verdict

APPROVED - The implementation is correct, follows Go best practices for mutex usage, and addresses the issue requirements.

## Code Review for PR #40 ### Summary This PR adds mutex protection to ConsistentHashRing for thread safety, addressing Issue #35. ### Analysis **Positive aspects:** 1. **Correct lock granularity**: Uses RWMutex with RLock for read operations (GetNode, GetNodes, IsEmpty) and Lock for write operations (AddNode, RemoveNode). 2. **Complete coverage**: All public methods that access shared state are protected. The private hash method correctly does not have its own lock since it is only called from within already-locked sections. 3. **Consistent use of defer**: All lock/unlock pairs use defer, ensuring locks are released even if early returns or panics occur. 4. **No deadlock risk**: The implementation does not call any external code while holding locks. 5. **Tests pass with race detector**: go test -race ./cluster/... passes. ### Verdict APPROVED - The implementation is correct, follows Go best practices for mutex usage, and addresses the issue requirements.

HugoNijhuis force-pushed issue-35-hashring-mutex from d33477c02a to 4666bb6503

2026-01-10 17:46:57 +00:00

Compare

HugoNijhuis merged commit 200dd5d551 into main

2026-01-10 17:47:53 +00:00

HugoNijhuis referenced this issue from a commit

2026-01-10 17:47:54 +00:00

Merge pull request '[Issue #35] Add mutex protection to ConsistentHashRing for thread safety' (#40) from issue-35-hashring-mutex into main

HugoNijhuis commented

2026-01-10 23:02:50 +00:00

Code Review for PR #40

Summary

This PR adds mutex protection to ConsistentHashRing for thread safety, addressing Issue #35.

Analysis

What was reviewed:

cluster/hashring.go - added sync.RWMutex and appropriate locking

Positive aspects:

Correct lock granularity: Uses RWMutex with RLock for read operations (GetNode, GetNodes, IsEmpty) and Lock for write operations (AddNode, RemoveNode). This allows concurrent reads while serializing writes - the optimal pattern for this use case.
Complete coverage: All public methods that access shared state are protected. The private hash method correctly does not have its own lock since it's only called from within already-locked sections.
Consistent use of defer: All lock/unlock pairs use defer, ensuring locks are released even if early returns or panics occur.
No deadlock risk: The implementation doesn't call any external code while holding locks, minimizing deadlock risk.
Tests pass with race detector: go test -race ./cluster/... passes.

Minor suggestions for future improvement (not blockers):

Concurrent stress tests: The existing tests don't specifically test concurrent access patterns. Consider adding a test that spawns multiple goroutines doing concurrent adds/removes/gets to validate thread safety under load.
Newline at EOF: Good - the newline at end of file was added (the old file was missing it).

Verdict

APPROVED - The implementation is correct, follows Go best practices for mutex usage, and addresses the issue requirements. All tests pass with the race detector enabled.

## Code Review for PR #40 ### Summary This PR adds mutex protection to `ConsistentHashRing` for thread safety, addressing Issue #35. ### Analysis **What was reviewed:** - `cluster/hashring.go` - added `sync.RWMutex` and appropriate locking **Positive aspects:** 1. **Correct lock granularity**: Uses `RWMutex` with `RLock` for read operations (`GetNode`, `GetNodes`, `IsEmpty`) and `Lock` for write operations (`AddNode`, `RemoveNode`). This allows concurrent reads while serializing writes - the optimal pattern for this use case. 2. **Complete coverage**: All public methods that access shared state are protected. The private `hash` method correctly does not have its own lock since it's only called from within already-locked sections. 3. **Consistent use of defer**: All lock/unlock pairs use `defer`, ensuring locks are released even if early returns or panics occur. 4. **No deadlock risk**: The implementation doesn't call any external code while holding locks, minimizing deadlock risk. 5. **Tests pass with race detector**: `go test -race ./cluster/...` passes. **Minor suggestions for future improvement (not blockers):** 1. **Concurrent stress tests**: The existing tests don't specifically test concurrent access patterns. Consider adding a test that spawns multiple goroutines doing concurrent adds/removes/gets to validate thread safety under load. 2. **Newline at EOF**: Good - the newline at end of file was added (the old file was missing it). ### Verdict **APPROVED** - The implementation is correct, follows Go best practices for mutex usage, and addresses the issue requirements. All tests pass with the race detector enabled.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: flowmade-one/aether#40