fix_: cancel ongoing requests when closing the ClientWithFallback #6536
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue of ongoing requests not being cancelled when the Client is closed by adding proper cancellation logic. Key changes include:
- Stopping all RPC clients by acquiring a mutex lock in rpc/client.go.
- Introducing a done channel and an atomic closed flag in ClientWithFallback (rpc/chain/client.go) to signal client closure.
- Adding tests in rpc/chain/client_test.go and circuitbreaker/circuit_breaker_test.go to verify the proper cancellation and panic behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/client.go | Updated Stop() to lock, iterate through, and close all clients. |
| rpc/chain/client_test.go | Added tests to verify behavior when client state is nil and when closing. |
| rpc/chain/client_health_test.go | Removed redundant expectation for context cancellation. |
| rpc/chain/client.go | Integrated done channel and atomic closed flag to handle closure. |
| circuitbreaker/circuit_breaker_test.go | Added test for ensuring a nil pointer panic on misuse of CommandResult. |
Jenkins BuildsClick to see older builds (43)
|
friofry
force-pushed
the
ab/issue-6525-nil-functors
branch
from
947e8fc to
0fb7d9c
Compare
| } | ||
|
|
||
| // Create a context that will be cancelled when either the parent context is done or the client is closed | ||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Instead of creating a done channel, we should save this cancel method to ClientWithFallback. And call it in the Stop method.
There was a problem hiding this comment.
I like this approach and prefer to store cancel functions in other types. And I understand that storing a context in a struct breaks the top-down cancel approach.
But here I was not sure how to manage/merge cancel functions for multiple calls. So storing a chan seems like a simpler solution. And it's not an antipattern.
There was a problem hiding this comment.
Well... technically done chan will not solve it, because a channel has only one receiver 🤷
So only one goroutine will be stopped
There was a problem hiding this comment.
Thinking about this... perhaps adding a WaitGroup to makeCall would be the right solution here. After cancelling all contexts, we should wait for all calls to finish and only then destroy the ClientWithFallback.
There was a problem hiding this comment.
Thinking about this... perhaps adding a
WaitGrouptomakeCallwould be the right solution here. After cancelling all contexts, we should wait for all calls to finish and only then destroy theClientWithFallback.
Thank you, fixed!
Well... technically
done chanwill not solve it, because a channel has only one receiver 🤷 So only one goroutine will be stopped
Here is an example with cancel()+chan approach for multiple parallel requests: https://go.dev/play/p/wZqHB0pasYr
rpc/chain/client.go
Outdated
| @@ -80,6 +81,9 @@ type ClientWithFallback struct { | |||
|
|
|||
| tag string // tag for the limiter | |||
| groupTag string // tag for the limiter group | |||
|
|
|||
| done chan struct{} // channel to signal client closure | |||
There was a problem hiding this comment.
hmm I get the feeling done and closed are doing here what an actual ctx should be doing. If we don't want to manage 2 separate contexts (one global for the whole client, one which comes with the request), there's stuff like this https://github.com/teivah/onecontext that will encapsulate it.
There was a problem hiding this comment.
I'm not sure how to use ctx, cancel := onecontext.Merge(ctx1, ctx2) in makeCall without having a parent context that was used in the Start method of rpc/client.go.
OR we can have an extra internal lifetime context to handle spawned gorutines cancellation. But then we need to add this edge case to the codesmells#2
friofry
force-pushed
the
ab/issue-6525-nil-functors
branch
from
ff453ef to
20919b4
Compare
* method is not called, since the ctx is cancelled before the method is called fixes #6525
friofry
force-pushed
the
ab/issue-6525-nil-functors
branch
from
20919b4 to
2a47d91
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6536 +/- ##
===========================================
- Coverage 60.31% 60.31% -0.01%
===========================================
Files 830 830
Lines 103642 103670 +28
===========================================
+ Hits 62514 62524 +10
+ Misses 33577 33572 -5
- Partials 7551 7574 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
|
friofry
force-pushed
the
ab/issue-6525-nil-functors
branch
from
94ed96f to
ebab628
Compare
The code uses three mechanisms to manage client closure:
closedprovides immediate rejection of new requests once the client is marked as closed. It ensures thatClose()is called only oncedone): When Close() is called, it closes the done channel which signals all ongoing operations to stop gracefully. This context cancellation approach ensures operations that are already in progress terminate properly.wg): The wait group tracks all active operations, allowingClose()to wait for them to complete before finalizing the shutdown process.Closes #6525
Closes #6576
Closes #6462