fix: indefinite function runs inside mutex lock #372
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 78.81% 79.41% +0.59%
==========================================
Files 82 91 +9
Lines 6461 6819 +358
==========================================
+ Hits 5092 5415 +323
- Misses 1115 1131 +16
- Partials 254 273 +19 ☔ View full report in Codecov by Sentry. |
Sidddddarth
requested review from
cisse21,
lvrach,
BonapartePC,
fracasula and
achettyiitr
The first one is to lock access to We only set |
stats/statsd.go
Outdated
| @@ -200,7 +211,13 @@ func (s *statsdStats) internalNewTaggedStat(name, statType string, tags Tags, sa | |||
| taggedClient = &statsdClient{samplingRate: samplingRate, tags: tagVals} | |||
| if s.state.connEstablished { | |||
| taggedClient.statsdMu.Lock() | |||
There was a problem hiding this comment.
There is no need to lock here given that we just created the client and it's not a key on any of the maps, right?
stats/statsd.go
Outdated
| @@ -297,9 +314,8 @@ type statsdClient struct { | |||
| } | |||
|
|
|||
| // ready returns true if the statsd client is ready to be used (not nil). | |||
| // | |||
| // statsdMu.RLock should be held when calling this method. | |||
There was a problem hiding this comment.
The same comment might be useful on skip().
| @@ -297,9 +312,8 @@ type statsdClient struct { | |||
| } | |||
|
|
|||
| // ready returns true if the statsd client is ready to be used (not nil). | |||
| // | |||
| // sc.statsdMu.RLock should be held when calling this method. | |||
There was a problem hiding this comment.
didn't understood, why are we removing lock from here and asking caller to hold lock? Isn't that same?
There was a problem hiding this comment.
Discussed offline, along with calling this function in skip() fn call, we are recording stats as well here. we would need to hold lock again before recording new stats. Holding multiple locks multiple time can slow application down so we removed lock from here.
Description
Pulled the indefinite function out of mutex locking.
Two locks:
statsdState.clientsLockstatsdClient.statsdMuThe first one is to lock access to
*statsdStats.state.pendingClients,*statsdStats.state.clientsand*statsdStats.state.client.The second is to lock access to
*statsdClient.*(statsd.Client).We attempt to set
*statsdStats.state.clientduring startup, and if that fails, a new goroutine(1) is spawned, which requires us to protect access. All other goroutines(n) only clone(read) from this client to create new clients for stats. The clients created in other goroutines(n) have a nil client if setup hasn't succeeded(which is set appropriately in 1 - which requires write access). The stat triggers then need read access for these cloned clients.Linear Ticket
Fixes PIPE-885
Security