search

SpartanJ / efsw

efsw is a C++ cross-platform file system watcher and notifier.
Other
645 stars 98 forks source link

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

Open maddanio opened 9 months ago

maddanio commented 9 months ago

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

SpartanJ commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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