-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks for that!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! 🤯
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did this work before without an atomic id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
f50df07
to
26b9223
Compare
Quality Gate failedFailed conditions |
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).