search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

Replace lock with CAS in standard meter stop #246

Closed sykesm closed 5 years ago

sykesm commented 5 years ago

Fixes #245

tsuna commented 5 years ago

I can confirm that this patch fixes the race.

tsuna commented 5 years ago

Ping. Can we get this merged please?

mihasya commented 5 years ago

Sorry about the delay, taking a look. Going to ping @deckarep since this looks related to the work from #232. @sykesm @tsuna @deckarep is this a missed spot from that PR? If so, I'll merge right away, but want to make sure I'm not missing something first. Thanks all!

tsuna commented 5 years ago

Not sure what you meant by "is this a missed spot from that PR?". If you're asking whether this fixes the race, then yes it does, I can confirm from my review of the code and my CI builds with -race passing with it.

mihasya commented 5 years ago

@tsuna the linked PR switched from using mutexes to using CAS. It is my understanding that this race was likely introduced at that point because one instance of mutex was not updated. I want to confirm that my understanding is correct. I don't look at this code very frequently any more, so I want to make sure I understand where the bug comes from, lest we introduce yet another one somewhere else.

tsuna commented 5 years ago

One code path was using a CAS, the other a mutex, that's no bueno. Either both should use a mutex or both should use a CAS. This fix removes the mutex altogether so that only CAS is used.

deckarep commented 5 years ago

Let me look stand by

deckarep commented 5 years ago

Going off my memory, yes this is related and the change makes sense...I probably did not catch this race...I also remember attempting to leave the stop logic as is.

This fix LGTM.

mihasya commented 5 years ago

👏 seems like we're all on the same page.