Feature/start stop #128
Feature/start stop #128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 57.36% 61.23% +3.87%
==========================================
Files 16 16
Lines 842 859 +17
==========================================
+ Hits 483 526 +43
+ Misses 334 308 -26
Partials 25 25
Continue to review full report at Codecov.
|
chrysom/client.go
Outdated
| c.observer.mu.Lock() | ||
| defer c.observer.mu.Unlock() | ||
|
|
||
| if c.observer.state == running { |
There was a problem hiding this comment.
If you're intending to use this method as an uber/fx Lifecycle hook, it should return an error when Start has already been called without previous call to Stop.
chrysom/client.go
Outdated
| @@ -354,28 +370,56 @@ func (c *Client) Start(ctx context.Context) error { | |||
| return ErrUndefinedIntervalTicker | |||
| } | |||
|
|
|||
| c.observer.mu.Lock() | |||
There was a problem hiding this comment.
You can certainly do this with a lock. However, the CAS pattern is way simpler for this. You don't need separate fields for a mutex and a state.
If you need some sort of control channel for the goroutine, it's pretty simple to just use an atomic.Value for that channel, which can also double as the atomic started/stopped state.
| } | ||
|
|
||
| c.observer.ticker.Stop() | ||
| c.observer.shutdown <- struct{}{} |
There was a problem hiding this comment.
It's better to actually close a shutdown channel. When Start is invoked, you create the channel. When Stop is invoked, you can close the channel and nil out the field.
The rationale here is that you don't want to miss state changes. When a channel is reused for shutdown, it can be possible for the previously started goroutine to miss the change and the currently started goroutine to receive the change. That's because there is no ordering to how goroutines receive data on channels.
joe94
force-pushed
the
feature/startStop
branch
from
ff104b9 to
4d5c38e
Compare
joe94
force-pushed
the
feature/startStop
branch
from
170849f to
ee3d358
Compare
|
SonarCloud Quality Gate failed. |
No description provided.