Add solution for Challenge 20 by hvijaycse#1468
Conversation
WalkthroughIntroduces a complete Go circuit breaker implementation with state machine transitions (Closed, Open, HalfOpen), thread-safe metrics tracking, configurable trip logic, and operation execution with context support in a single new file. Changes
Sequence DiagramsequenceDiagram
actor Client
participant CB as CircuitBreaker
participant Op as Operation
Client->>CB: Call(ctx, operation)
CB->>CB: canExecute()
alt Circuit Closed or Ready
CB->>Op: executeOperationWithContext()
Op-->>CB: result/error
alt Success
CB->>CB: recordSuccess()
CB->>CB: setState (HalfOpen→Closed)
else Failure
CB->>CB: recordFailure()
CB->>CB: shouldTrip()
alt Trip Triggered
CB->>CB: setState (any→Open)
else HalfOpen Limit Exceeded
CB->>CB: setState (HalfOpen→Open)
end
end
else Circuit Open (Timeout Not Reached)
CB-->>Client: ErrCircuitBreakerOpen
else Circuit Open (Ready to HalfOpen)
CB->>CB: setState (Open→HalfOpen)
CB->>Op: executeOperationWithContext()
else HalfOpen (At MaxRequests)
CB-->>Client: ErrTooManyRequests
end
CB-->>Client: result/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
challenge-20/submissions/hvijaycse/solution-template.go (5)
47-47:Intervalfield is declared but never used.The
Intervalfield (described as "Statistical window for closed state") is defined inConfigbut the implementation never uses it to reset metrics periodically. If this is intentional for a simplified implementation, consider removing it or adding a comment. Otherwise, you may need to implement interval-based metric resets.
211-216: Minor: incrementhalfOpenRequestsonly when allowing the request.Currently,
halfOpenRequestsis incremented before the limit check, so rejected requests also inflate the counter. While functionally correct (the counter resets on state transitions), incrementing only upon successful admission is cleaner.♻️ Suggested refactor
case StateHalfOpen: - cb.halfOpenRequests++ if cb.halfOpenRequests > cb.config.MaxRequests { return ErrTooManyRequests } + cb.halfOpenRequests++ return nilNote: With this change, the condition should use
>=instead of>:case StateHalfOpen: - if cb.halfOpenRequests > cb.config.MaxRequests { + if cb.halfOpenRequests >= cb.config.MaxRequests { return ErrTooManyRequests } cb.halfOpenRequests++ return nil
237-238: Remove commented-out code.These lines appear to be leftover from development and should be removed.
🧹 Proposed cleanup
if cb.state == StateHalfOpen { cb.setState(StateClosed) - // cb.state = StateClosed - // cb.lastStateChange = time.Now() }
259-260: Remove commented-out code.These leftover comments should be removed. Note that line 259 has an incorrect comment (
StateClosedinstead ofStateOpen).🧹 Proposed cleanup
if cb.state == StateHalfOpen { cb.setState(StateOpen) - // cb.state = StateClosed - // cb.lastStateChange = time.Now() } else if cb.state == StateClosed && cb.shouldTrip() { cb.setState(StateOpen) - // cb.state = StateOpen - // cb.lastStateChange = time.Now() - }Also applies to: 263-264
274-276: Remove commented-out code.The commented-out lock code should be removed. Note that it had a bug (
RLockpaired withUnlockinstead ofRUnlock), so it's good this was commented out.🧹 Proposed cleanup
func (cb *circuitBreakerImpl) shouldTrip() bool { // Use the ReadyToTrip function from config with current metrics - // cb.mutex.RLock() - // defer cb.mutex.Unlock() return cb.config.ReadyToTrip(cb.metrics) }func (cb *circuitBreakerImpl) isReady() bool { // Check if enough time has passed since last state change (Timeout duration) - // cb.mutex.RLock() - // defer cb.mutex.Unlock() return time.Since(cb.lastStateChange) > cb.config.Timeout }Also applies to: 285-287
| // function to execute the operation while also keeping a check on context | ||
| func executeOperationWithContext(ctx context.Context, operation func() (interface{}, error)) (interface{}, error) { | ||
| resultChan := make(chan result) | ||
|
|
||
| go func() { | ||
| data, err := operation() | ||
| resultChan <- result{res: data, err: err} | ||
| }() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case out := <-resultChan: | ||
| return out.res, out.err | ||
| } | ||
| } |
There was a problem hiding this comment.
Goroutine leak when context is cancelled.
If ctx.Done() fires before the operation completes, this function returns immediately, but the spawned goroutine continues running. When the operation eventually finishes, the goroutine blocks forever trying to send on the unbuffered resultChan since no receiver exists.
🛠️ Proposed fix using a buffered channel
func executeOperationWithContext(ctx context.Context, operation func() (interface{}, error)) (interface{}, error) {
- resultChan := make(chan result)
+ resultChan := make(chan result, 1)
go func() {
data, err := operation()
resultChan <- result{res: data, err: err}
}()
select {
case <-ctx.Done():
return nil, ctx.Err()
case out := <-resultChan:
return out.res, out.err
}
}A buffered channel of size 1 allows the goroutine to complete its send even if no receiver is waiting, preventing the leak. Note that the operation itself will still run to completion—this fix only prevents the goroutine from blocking indefinitely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // function to execute the operation while also keeping a check on context | |
| func executeOperationWithContext(ctx context.Context, operation func() (interface{}, error)) (interface{}, error) { | |
| resultChan := make(chan result) | |
| go func() { | |
| data, err := operation() | |
| resultChan <- result{res: data, err: err} | |
| }() | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case out := <-resultChan: | |
| return out.res, out.err | |
| } | |
| } | |
| // function to execute the operation while also keeping a check on context | |
| func executeOperationWithContext(ctx context.Context, operation func() (interface{}, error)) (interface{}, error) { | |
| resultChan := make(chan result, 1) | |
| go func() { | |
| data, err := operation() | |
| resultChan <- result{res: data, err: err} | |
| }() | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case out := <-resultChan: | |
| return out.res, out.err | |
| } | |
| } |
2 participants
Challenge 20 Solution
Submitted by: @hvijaycse
Challenge: Challenge 20
Description
This PR contains my solution for Challenge 20.
Changes
challenge-20/submissions/hvijaycse/solution-template.goTesting
Thank you for reviewing my submission! 🚀