Skip to content
This repository was archived by the owner on Apr 1, 2025. It is now read-only.

Conversation

@sykesm
Copy link
Contributor

@sykesm sykesm commented Oct 1, 2018

Fixes #245

Fixes #245

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
@tsuna
Copy link

tsuna commented Oct 11, 2018

I can confirm that this patch fixes the race.

@tsuna
Copy link

tsuna commented Oct 15, 2018

Ping. Can we get this merged please?

@mihasya
Copy link
Collaborator

mihasya commented Oct 15, 2018

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
Copy link

tsuna commented Oct 15, 2018

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
Copy link
Collaborator

mihasya commented Oct 15, 2018

@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
Copy link

tsuna commented Oct 15, 2018

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
Copy link

Let me look stand by

@deckarep
Copy link

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
Copy link
Collaborator

mihasya commented Oct 16, 2018

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

@mihasya mihasya merged commit 3113b84 into rcrowley:master Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants