Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix data race in CondVar timeout #5456

Merged

Conversation

patrickdupuis
Copy link
Contributor

Purpose and Motivation

This PR fixes a data race that can happen when a CondVar is signaled immediately as it is timing out. The problem is easy to reproduce:

(
var cond = CondVar();
var test = false;
var waitTime = 1.0;

fork {
    waitTime.wait;
    cond.signalOne;
};

fork {
    cond.waitFor(waitTime, { test });
    "waitFor timed out".postln;
};
)

An error is thrown because the time out mechanism is trying to wake a thread that has already been woken by the signal. this.prRemoveWaitingThread(waitingThread); returns nil in this case, so this.prWakeThread(wokenThread); throws an error having received a nil value.

It doesn't look like this change has any adverse effects. TestCondVar.sc is still passing. I tried to write a test for this issue, but couldn't figure out a way to write one using assertNoException. If anyone knows how I can write a test for this, please let me know and I'll add it to this PR.

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@patrickdupuis patrickdupuis added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library labels May 23, 2021
@patrickdupuis patrickdupuis added this to the 3.12.0 milestone May 23, 2021
@patrickdupuis
Copy link
Contributor Author

@joshpar @dyfer this is ready to be merged

@dyfer dyfer merged commit b4e779a into supercollider:develop May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants