Skip to content

Commit

Permalink
Implement notification loop safeguard in ofParameter<void>
Browse files Browse the repository at this point in the history
  • Loading branch information
@eduardfrigola
eduardfrigola committed Jul 23, 2020
1 parent 29f5ede commit 7bbf058
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 35 deletions.
88 changes: 54 additions & 34 deletions libs/openFrameworks/types/ofParameter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,50 +105,70 @@ ofParameter<void>& ofParameter<void>::set(const std::string & name){
}

void ofParameter<void>::trigger(){
ofNotifyEvent(obj->changedE,this);
// Notify all parents, if there are any.
if(!obj->parents.empty())
// If the object is notifying its parents, just set the value without triggering an event.
if(!obj->bInNotify)
{
// Erase each invalid parent
obj->parents.erase(std::remove_if(obj->parents.begin(),
obj->parents.end(),
[this](const std::weak_ptr<ofParameterGroup::Value> & p){ return p.expired(); }),
obj->parents.end());

// notify all leftover (valid) parents of this object's changed value.
// this can't happen in the same iterator as above, because a notified listener
// might perform similar cleanups that would corrupt our iterator
// (which appens for example if the listener calls getFirstParent on us)
for(auto & parent: obj->parents){
auto p = parent.lock();
if(p){
p->notifyParameterChanged(*this);
// Mark the object as in its notification loop.
obj->bInNotify = true;

// Notify any local subscribers.
ofNotifyEvent(obj->changedE, this);

// Notify all parents, if there are any.
if(!obj->parents.empty())
{
// Erase each invalid parent
obj->parents.erase(std::remove_if(obj->parents.begin(),
obj->parents.end(),
[this](const std::weak_ptr<ofParameterGroup::Value> & p){ return p.expired(); }),
obj->parents.end());

// notify all leftover (valid) parents of this object's changed value.
// this can't happen in the same iterator as above, because a notified listener
// might perform similar cleanups that would corrupt our iterator
// (which appens for example if the listener calls getFirstParent on us)
for(auto & parent: obj->parents){
auto p = parent.lock();
if(p){
p->notifyParameterChanged(*this);
}
}
}
obj->bInNotify = false;
}
}

void ofParameter<void>::trigger(const void * sender){
ofNotifyEvent(obj->changedE,sender);
// Notify all parents, if there are any.
if(!obj->parents.empty())
// If the object is notifying its parents, Do not trigger the event.
if(!obj->bInNotify)
{
// Erase each invalid parent
obj->parents.erase(std::remove_if(obj->parents.begin(),
obj->parents.end(),
[this](const std::weak_ptr<ofParameterGroup::Value> & p){ return p.expired(); }),
obj->parents.end());

// notify all leftover (valid) parents of this object's changed value.
// this can't happen in the same iterator as above, because a notified listener
// might perform similar cleanups that would corrupt our iterator
// (which appens for example if the listener calls getFirstParent on us)
for(auto & parent: obj->parents){
auto p = parent.lock();
if(p){
p->notifyParameterChanged(*this);
// Mark the object as in its notification loop.
obj->bInNotify = true;

// Notify any local subscribers.
ofNotifyEvent(obj->changedE, this);

This comment has been minimized.

Sign in to view

Copy link
@eduardfrigola

eduardfrigola Mar 7, 2022

Author Member

Now I see that here this should be ofNotifyEvent(obj->changedE, sender);.
As it is the only place where the two implementations are different.

// Notify all parents, if there are any.
if(!obj->parents.empty())
{
// Erase each invalid parent
obj->parents.erase(std::remove_if(obj->parents.begin(),
obj->parents.end(),
[this](const std::weak_ptr<ofParameterGroup::Value> & p){ return p.expired(); }),
obj->parents.end());

// notify all leftover (valid) parents of this object's changed value.
// this can't happen in the same iterator as above, because a notified listener
// might perform similar cleanups that would corrupt our iterator
// (which appens for example if the listener calls getFirstParent on us)
for(auto & parent: obj->parents){
auto p = parent.lock();
if(p){
p->notifyParameterChanged(*this);
}
}
}
obj->bInNotify = false;
}
}

Expand Down
5 changes: 4 additions & 1 deletion libs/openFrameworks/types/ofParameter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,14 +1054,17 @@ class ofParameter<void>: public ofAbstractParameter{
class Value{
public:
Value()
:serializable(false){}
:bInNotify(false)
,serializable(false){}

Value(std::string name)
:name(name)
,bInNotify(false)
,serializable(false){}

std::string name;
ofEvent<void> changedE;
bool bInNotify;
bool serializable;
std::vector<std::weak_ptr<ofParameterGroup::Value>> parents;
};
Expand Down

6 comments on commit 7bbf058

@roymacdonald
Copy link

Choose a reason for hiding this comment

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

This looks good to me.

It is kind of an edge case but it is a case.

Is there any threading issue with this change? wouldn't it be safer if bInNotify was atomic?

@eduardfrigola
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Roy,
The code I used was a "copy" of what is already implemented in ofParameter template.
https://github.com/openframeworks/openFrameworks/blob/c5ef11916839a2e857d0f8a75abb408bf25cb0cb/libs/openFrameworks/types/ofParameter.h#L634

I only added the same code to the template specialisation ofParameter.
As ofParameter itself is not thread safe (if I'm not mistaken) this should not be needed, as this function is expected to only be called by itself.

@roymacdonald
Copy link

Choose a reason for hiding this comment

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

I see. cool then.
It is then that the bIsNotify check was missing in the ofParameter rather than a new feature.
I would say that this is fine to merge. @ofTheo
More over, it does not seem that it would introduce side effects.

The only thing that kinda bothers me, but it does not belong to this PR really is the copy/pasting of the code for notifying parents. But that should go on a separate PR.

@eduardfrigola
Copy link
Member Author

@eduardfrigola eduardfrigola commented on 7bbf058 Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I agree with you regarding duplicate code.
Will calling trigger(this) inside ofParameter<void>::trigger(const void * sender) cause any problems?

Don't you think should be better to make a PR trying to reduce the code copied that is already in there. And when that get's merged, do the PR with the change proposed here?

@eduardfrigola
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, trying to reuse void ofParameter<ParameterType>::eventsSetValue(const ParameterType & v)

Would mean to put some of the code in there into a function, to then reuse just some parts of it, wouldn't that put more complexity and it becomes worse to try avoid copy pasting?
I got some times in the same situation, where, for just a niche case specialisation, trying to make the code usable for all the cases, it got everything a lot messier.

Anyway, I think I can't even agree with myself, so i can give a try into making the code in the templated version usable for ofParameter<void> and we can see if it is actually better or worse.

@roymacdonald
Copy link

@roymacdonald roymacdonald commented on 7bbf058 Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi

Anyway, I think I can't even agree with myself, so i can give a try into making the code in the templated version usable for ofParameter<void> and we can see if it is actually better or worse.

I don't mean to make the templated version to be able to use void. What is there is quite neat. As you say, it would just introduce more complexity.

Would mean to put some of the code in there into a function, to then reuse just some parts of it, wouldn't that put more complexity and it becomes worse to try avoid copy pasting?

I mean just putting the notification to parents part into a function
https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/types/ofParameter.cpp#L112-L127
It is the same for all the times it is repeated, although the Value type is different thus the need to make it templated, and ..... I would forget about it right now.

But, as it is now it works, so why spending time trying to fix it.

Will calling trigger(this) inside ofParameter::trigger(const void * sender) cause any problems?
Not sure, its been there and aparently without creating problems.

Please sign in to comment.