Add conditional callback, use atomic handle_id and add some tests #2307
Conversation
| std::lock_guard<std::mutex> guard(mutex_); | ||
| if (handle_ptr && handle_ptr->has_value()) { | ||
| if (!handle_set.insert(handle_ptr->value()).second) { | ||
| EXPECT_TRUE(false); // handles are not unique |
There was a problem hiding this comment.
To make the check fail on main i had to increase the thread count to over 100k and in the subscribe() call comment out the check_removals() - not sure why, maybe it was causing some synchronization as a side effect on the rest of the code. In that setting, i could reproduce the failures on main and not on this PR. Another way I checked it was adding a method to pull the _last_id from the callbacklist class, then comparing it with the threadcount, on main (under the conditions mentioned above) it was usually less than the thread_count(-1) and not on this PR.
| std::lock_guard<std::mutex> guard(mutex_); | ||
| if (handle_ptr && handle_ptr->has_value()) { | ||
| if (!handle_set.insert(handle_ptr->value()).second) { | ||
| EXPECT_TRUE(false); // handles are not unique |
|
|
||
| // We need to return a handle, even if the callback is nullptr to | ||
| // unsubscribe. That's fine, the handle just won't remove anything | ||
| // when/if used later. | ||
| auto handle = Handle<Args...>(_last_id++); | ||
| auto handleId = _last_id.fetch_add(1, std::memory_order_relaxed); |
There was a problem hiding this comment.
How did this work before without an atomic id?
There was a problem hiding this comment.
I think the issue only happens if you add a ton of subscriptions from different threads simultaneously - kind of a rare scenario, so we didn’t see it earlier.
|
I wonder if/how I can re-use this for timeout and call every subscription. I basically have similar problems there. |
I haven’t seen it for a while, wouldn’t it make sense that all of those also use this callbacklist class? don’t they, I guess they were implemented before we added this class. |
|
I think it might make sense, indeed. They also use the stupid |
|
I will do that in the future, don't want to hold up this PR. |
|
@devbharat do you mind fixing style? |
julianoes
force-pushed
the
fix/cb_list
branch
2 times, most recently
from
f50df07
to
26b9223
Compare
|
|
I was just coming to fix style, thanks for doing it already. seems like some other test is failing, but looks unrelated to the PR. |
|
Oh, thought I had merged it already. Funny. Anyway, it's all good. SonarCloud is just a bit picky. |
Fixes #2215
Adds
I needed to add these to create something similar to the MavlinkMessageHandler class that uses a map of msgid to callbacklists, but crucially it allows adding subscriptions from within callbacks (if you get this mavlink message, send this message and subscribe to this mavlink message kind of chained workflow), which meant (in addiiton to deferred subs in cblist) the execution of the callbacklist from MavlinkMessageHandler needed to be outside the lock (comparison here).