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

Remove CriticalSection, replace with std::mutex #2765

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

mvf
Copy link
Collaborator

@mvf mvf commented Oct 2, 2020

Didn't seem to be used recursively. If it is after all, that's a bug
that needs fixing. If it can't be fixed right away, there's always
std::recursive_mutex to restore the old behavior.

Didn't seem to be used recursively. If it is after all, that's a bug
that needs fixing. If it can't be fixed right away, there's always
std::recursive_mutex to restore the old behavior.
@mvf mvf force-pushed the pr/critsec-bgone branch from e505d74 to edd4e21 Compare October 2, 2020 05:17
@mvf mvf merged commit cd2c84c into surge-synthesizer:main Oct 2, 2020
@mvf mvf deleted the pr/critsec-bgone branch October 2, 2020 05:28
@baconpaul
Copy link
Collaborator

Oooh would have been good to chat since there are recursive uses and I’ve never trapped them.

#1249 Gives you one example

What happens now if we do recourse? Deadlock?

@@ -51,6 +51,10 @@
#include <stack>
#include <unordered_map>

#if WINDOWS
#include <windows.h>
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the definitions of SYSTEMTIME/GetSystemTime. Before, windows.h was included implicitly via SurgeStorage.hCriticalSection.h.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
We should move that to std::chrono one day

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we actually didn't need the time at all so just cleaned it up and pushed

@mvf
Copy link
Collaborator Author

mvf commented Oct 2, 2020

What happens now if we do recurse? Deadlock?

Yes, which is hopefully easy to debug. Recursive mutexes are a hack and especially bad in audio applications. It seems that the current known recursive use it not intentional though. I'll look into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants