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

Context callback changes and device mutex removal #12275

Merged
merged 5 commits into from Oct 11, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Oct 10, 2023

This is an API-breaking change:

  • a context is created
  • rs2_set_devices_changed_callback is called
  • the context is destroyed with rs2_delete_context

Before: as long as the internal context object was held alive (by a device etc.), the callback was still callable, possibly even when unintended (requiring the user to set an "empty" callback as a workaround)

Now: when delete-context is called, the callback is unsubscribed and no longer happens. I.e., you now have to hold the context if you want callbacks from it. This is the correct behavior.

Changes:

  • simplify context callback handling; only a single function remains, on_device_changes()
  • make use of rsutils::signal (we handle multiple callbacks), and remove mutex in context
  • simpler & more robust device::is_valid() implementation, without a mutex around it

Everything together solves the deadlock in #11933. A test will be added separately to regressions.

Tracked on [RSDSO-19304]

@maloel maloel requested a review from Nir-Az October 10, 2023 13:20
@@ -9,6 +9,11 @@
#include "dds/rsdds-device-factory.h"
#endif

#include <librealsense2/hpp/rs_types.hpp> // rs2_devices_changed_callback
#include <librealsense2/rs.h> // RS2_API_VERSION_STR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the comment is needed.
Tomorow if we add another used and it compiles, we will need to update this comment?
Will not happen :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it helps -- at least for this version of the code.
But you're right, it likely won't get updated.
Hopefully I'm getting the context to be focused-enough so that it'll never have to be updated again...

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maloel maloel merged commit 0ecde9f into IntelRealSense:development Oct 11, 2023
17 checks passed
@maloel maloel deleted the context-callbacks branch October 11, 2023 11:26
@maloel maloel mentioned this pull request Oct 25, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants