Concurrency primitives need concurrent tests #6274
Conversation
Honestly I think it's shocking that this was ever approved. The good news though is that it does seem to work. I also took the opportunity to remove the wildly unnecessary test suite use.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| lock.Unlock() | ||
| } | ||
|
|
||
| func BenchmarkLock(b *testing.B) { | ||
| l := NewMutex() | ||
| ctx := context.Background() | ||
| for n := 0; n < b.N; n++ { | ||
| l.Lock(ctx) //nolint:errcheck | ||
| l.Unlock() //nolint:staticcheck | ||
| _ = l.Lock(ctx) |
There was a problem hiding this comment.
The lockInternal could be optimized by keeping an internal state in an atomic variable. When there's noone holding the lock (state=0) it can be a lightweight operation bypassing the goroutine + channel wait.
There was a problem hiding this comment.
yea, I've got a different PR for making changes. this is just setting groundwork for "verify that this thing actually works"
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Spotted while looking into other racy code.
Honestly I think it's shocking that this was ever approved.
The good news though is that it does seem to work.
I also took the opportunity to remove the wildly unnecessary testify/suite, and add some comparative benchmarks:
At the moment I'm not sure if this is an acceptable cost or not tbh (though it feels like "no").