dacap/observable

thread safety bug

NoAvailableAlias opened this issue · 7 comments

Hello,

I have added your implementation of signals to this benchmark project here. However, it appears that there is either a bug in dacap/observable in regards to thread safety or my integration is missing something.

I'm currently working on adding more implementations of signals and slots to this benchmark project so I don't currently have the time to investigate the issue further. If you find anything / resolve anything update this bug and I'll get those changes integrated.

(on a side note I had to modify some file names/includes in order for dacap/observable to play nice in an environment full of signal slot implementations)

Matt M.

dacap commented

Hi Matt, actually I'm not aware about some thread safety issues but I'm pretty sure they might exist.

There are some tests using -fsanitize=thread and I've fixed some problems before about this, but maybe I could add new tests with to check threads issues.

Can you pointing me out about the code that is creating thread, connecting, and calling signals in your project?

Sure thing, here is the specific method that I run all thread safe signal libraries against.

The benchmarks were designed to abstract the benchmark test method away from all other signal libraries using templates. "Foo" in this case refers to your specific benchmark class "Dob" in benchmark/hpp/benchmark_dob.hpp

(also Subject refers to the "Signal" alias defined in benchmark_dob.hpp)

dacap commented

I reproduced some issues, I'll post some comments/news/fixed tomorrow about this. Thanks for your help @NoAvailableAlias.

dacap commented

@NoAvailableAlias check the latest commit, the crash should be fixed now. Thank you very much for reporting this and your work on signal-slot-benchmarks! Tell me if the problem continues.

In a future I'd love to improve the performance of this library too.

dacap commented

(I'm checking the automated tests in CI servers and someones are failing. So there are other bugs in the code.)

dacap commented

@NoAvailableAlias finally the commit 873cd8d should fix this issue.

Just finished pulling your changes and have updated your results here. Looks like everything is good to go. Nice work resolving this issue so fast btw!