palacaze / sigslot

A simple C++14 signal-slots implementation
MIT License
709 stars 97 forks source link

Thread Safety issue #20

Closed broken-bytes closed 3 years ago

broken-bytes commented 3 years ago

When emitting the signal from a thread that was not the creator of the signal object, the code crashes.

The code line that fails is the mutex lock function.

void lock() {
    _Check_C_return(_Mtx_lock(_Mymtx()));
}

Even when no slots are connected to the signal at all, the behaviour can be reproduced.

This simple test app crashes as well:

sigslot::signal<int> sig;

void Run() {
 while(true) {
 sig(1);
}
}

void main() {
std::thread t(Run);
while(true) {}
}

The compiler used is MSCV in VS 2019. Latest Windows 10 build.

palacaze commented 3 years ago

Thank you for the report. I cannot reproduce the bug on GNU/Linux with the following program. The thread sanitizers do not detect any data race.

#include <thread>
#include <sigslot/signal.hpp>

sigslot::signal<int> sig;

void Run() {
    unsigned long i = 0;
    while (i++ < 10'000'000) {
        sig(1);
    }
}

int main() {
    std::thread t(Run);
    t.join();
}

I do not have access to Windows 10 right now so I cannot test it myself, and I have yet to setup a CI workflow through Github Actions.

Can you provide the full code for your custom Lockable?

broken-bytes commented 3 years ago

I have simplified my example.

In the actual codebase, I am using an instance of a class that is initialized as a weak_ptr. When not using the weak_ptr but the object directly it did work.

When I called weak_ptr->lock(); the signal crashed with a mutex exception.

My guess is that locking the weak_ptr causes the signal to fail. Unfortunately I did change the codebase so there no longer is a weak_ptr involved.

palacaze commented 3 years ago

As a I no way of reproducing your code sample, is it ok to close this issue?

broken-bytes commented 3 years ago

I'd like to see if I can find the code in an earlier commit. That's probably tomorrow. I would notify you either way, so would it be okay to keep it open for a day ?

palacaze commented 3 years ago

Yes this is totally fine.

broken-bytes commented 3 years ago

Sorry for the late reply. The issue was a memory leak that was caused by a leaking resource, after further testing I can safely say the thread safety works fine.

palacaze commented 3 years ago

Thank you for taking the time to check further.