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

potential data race in emission #20

Open
NoAvailableAlias opened this issue Jan 19, 2019 · 2 comments
Open

potential data race in emission #20

NoAvailableAlias opened this issue Jan 19, 2019 · 2 comments

Comments

@NoAvailableAlias
Copy link

NoAvailableAlias commented Jan 19, 2019

Hello, recently I have been working on the next release of nano-signal-slot for some time now and have encountered a data race issue with my current iteration. Curious how others solved it I started looking into other thread safe implementations. Perusing nod I noticed the exact same signature of the issue exists in nod.

The signature is:

for (auto slot : copied_slots)
{
    if (slot)
        slot(args...)
}

The issue is the lock for emission is released as soon as the copied_slots is created. Due to this there is now a "potential data race" where the slot target "could" be destructed before being called in the emission loop.

Edit: Also my first attempt at resolving this is the same as how you currently test your shared disconnector in destruction. However, this would be UB now instead of a data race as accessing a class in destruction is UB.

@dioss-Machiel
Copy link

I think there are two solutions:

  1. Hold the lock during signal emission, recursive mutex can be used if slots need to modify the list.
  2. Make the slot connection class the owner of the std::function and hold a weak_ptr to the connections in the signal class.

@NoAvailableAlias
Copy link
Author

NoAvailableAlias commented Nov 22, 2019

Recursive mutex would only allow for reentrancy, the slot emission race would still exist?

Your second point is what my previous Edit: was about.

However, after a year absence, I'm pretty sure there would be no UB as destruction is blocked by its own locked weak_ptr (heap owned reference counter). The block to destruction would only occur if an emission is currently taking place in a different thread as the tracked observer is being destructed.

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

No branches or pull requests

2 participants