Skip to content

Conversation

@maloel
Copy link
Contributor

@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
#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
Contributor 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
@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.

2 participants