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

Thread-safety for device descriptor sending in async plugins. #423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rpavlik
Copy link
Member

@rpavlik rpavlik commented Apr 21, 2016

I think I hit this in early revisions of the vive plugin - it seems like the original implementation of the device descriptor sending code assumed you'd either only send it once, or you'd be a sync plugin and send from the main thread: there was no send-guard or equiv. locking around the descriptor update but that's definitely not something you want to do in a non-main thread since it can trigger a send.

This hasn't been tested, don't merge it until it has been.

@@ -116,6 +116,8 @@ bool OSVR_DeviceTokenObject::releaseObject(void *obj) {

void OSVR_DeviceTokenObject::setDeviceDescriptor(
std::string const &jsonString) {
auto guard = getSendGuard();
guard->lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call unlock() or does a destructor take care of that? (I gave up tracing through the code after 10 minutes.)

@rpavlik
Copy link
Member Author

rpavlik commented Apr 23, 2016

It's a guard class - so raii handles the unlock. It just needs a manual lock because it's polymorphic and the actual guard, if applicable, is returned, not a lockable object. (Tracing it shouldn't be that hard, think it's just guard interface in util).

@godbyk
Copy link
Contributor

godbyk commented Apr 23, 2016

The GuardInterface destructor was empty, which is why I asked.

@rpavlik
Copy link
Member Author

rpavlik commented Apr 26, 2016

Well, right, the base class is, since sync devices don't need to pay
locking costs. If you have an async device token, your send guard should be
derived from guard interface but actually do something, iirc.

On Sat, Apr 23, 2016, 3:08 PM Kevin Godby [email protected] wrote:

The GuardInterface destructor was empty, which is why I asked.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#423 (comment)

Ryan A. Pavlik, Ph.D.
Senior Software Engineer
Sensics, Inc.
www.sensics.com

@rpavlik rpavlik added this to the Pull request cleanup milestone Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants