Skip to content

Commit

Permalink
better scoping of thread gaurds
Browse files Browse the repository at this point in the history
  • Loading branch information
ianhi committed Feb 24, 2022
1 parent 9c39070 commit 3ba9d48
Showing 1 changed file with 10 additions and 5 deletions.
15 changes: 10 additions & 5 deletions DeviceAdapters/DemoCamera/DemoCamera.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1307,15 +1307,17 @@ void CDemoCamera::SlowPropUpdate()
// in a thread
long delay; GetProperty("AsyncPropertyDelayMS", delay);
CDeviceUtils::SleepMs(delay);
MMThreadGuard g(asyncFollowerLock_);
asyncFollower = asyncLeader;
{
MMThreadGuard g(asyncFollowerLock_);
asyncFollower = asyncLeader;
}
OnPropertyChanged("AsyncPropertyFollower", asyncFollower.c_str());

This comment has been minimized.

Copy link
@marktsuchida

marktsuchida Feb 24, 2022

Member

You need to copy asyncFollower (which should be asyncFollower_, BTW) to a temporary local variable while holding the mutex.

std::string followerValue;
{
   MMThreadGuard g(asyncFollowerLock_);
   asyncFollower = asyncLeader;
   followerValue = asyncFollower;
}
OnPropertyChanged("AsyncPropertyFollower", followerValue.c_str());

This comment has been minimized.

Copy link
@ianhi

ianhi Feb 24, 2022

Author Contributor

This is because as soon I leave the braces the value asyncFollower is no longer protected?

}

int CDemoCamera::OnAsyncFollower(MM::PropertyBase* pProp, MM::ActionType eAct)
{
MMThreadGuard g(asyncFollowerLock_);
if (eAct == MM::BeforeGet){
MMThreadGuard g(asyncFollowerLock_);
pProp->Set(asyncFollower.c_str());
}
// no AfterSet as this is a readonly property
Expand All @@ -1324,13 +1326,16 @@ int CDemoCamera::OnAsyncFollower(MM::PropertyBase* pProp, MM::ActionType eAct)

int CDemoCamera::OnAsyncLeader(MM::PropertyBase* pProp, MM::ActionType eAct)
{
MMThreadGuard g(asyncLeaderLock_);
if (eAct == MM::BeforeGet){
MMThreadGuard g(asyncLeaderLock_);

This comment has been minimized.

Copy link
@marktsuchida

marktsuchida Feb 24, 2022

Member

You need to use a single mutex, not two separate ones for leader and follower. Otherwise there is a race between this function and SlowPropUpdate(), which accesses asyncLeader on a different thread.

This comment has been minimized.

Copy link
@ianhi

ianhi Feb 24, 2022

Author Contributor

maybe the SlowPropUpdate should just take an argument of the value at the time of dispatch?

pProp->Set(asyncLeader.c_str());
}
if (eAct == MM::AfterSet)
{
pProp->Get(asyncLeader);
{
MMThreadGuard g(asyncLeaderLock_);
pProp->Get(asyncLeader);
}
fut_ = std::async(std::launch::async, &CDemoCamera::SlowPropUpdate, this);
}
return DEVICE_OK;
Expand Down

0 comments on commit 3ba9d48

Please sign in to comment.