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

use c++11 primitives for threads and locks #170

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maddanio
Copy link

No description provided.

@maddanio
Copy link
Author

ah, linux and windows builds fail, will look into it

@SpartanJ
Copy link
Owner

I'm gonna start using this PR branch locally for some time and if everything goes well I'll merge it, but I already copied and updated the branch to the local repo here, so'll merge that one when ready. Thanks for the collaboration.

@SpartanJ
Copy link
Owner

It seems that this branch has breaking changes. Implementation is broken, at least when tested with my application. My guess is that we are not using recursive mutexes and before they were recursive. Currently it's generating dead-locks all over the place for me. I'll investigate later.

@maddanio
Copy link
Author

Hmm, needing recursive mutexes is usually a code smell, so i am not sure its a good default. But thats just me maybe. Could also be an option though. There is a single place the mutex type is chosen. If it was originally recursive it would be proper to use std::recursive_mutex in this PR though, so i can do that if desired

@SpartanJ
Copy link
Owner

Hmm, needing recursive mutexes is usually a code smell, so i am not sure its a good default.

As far as I remember (and briefly tested) they're required in order to properly function at least for the inotify implementation. So this cannot be optional at least for some implementations, I'll leave the mutex as recursive for now, since I already confirmed that it works. Also I think this should work as a black-box for the consumer, they don't need to care about the implementation if possible. The ideal solution would be to use the adequate mutex on each occasion, meanwhile we know for sure that recursive mutexes will work fine since it respects the previous working implementation. I'll test with recursive mutexes and see how it behaves. I don't have any hurry for merging this so I'll take my time, better safe than sorry. Thanks again

@maddanio
Copy link
Author

I think this pr should use recursive mutex. Reducing the need for recursion should be another PR i would say. Will change in a minute

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