efsw icon indicating copy to clipboard operation
efsw copied to clipboard

use c++11 primitives for threads and locks

Open maddanio opened this issue 1 year ago • 6 comments

maddanio avatar Nov 22 '23 21:11 maddanio

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

maddanio avatar Nov 23 '23 07:11 maddanio

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 avatar May 22 '24 17:05 SpartanJ

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.

SpartanJ avatar May 23 '24 14:05 SpartanJ

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

maddanio avatar May 23 '24 17:05 maddanio

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

SpartanJ avatar May 23 '24 18:05 SpartanJ

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

maddanio avatar May 25 '24 07:05 maddanio