Bug(s) in EventCatcher

Hi,

I need some advice on the following:

1 Result handler( typename Function::arg1_type a1,
2 typename Function::arg2_type a2 ) {
3 if ( !this->enabled )
4 return detail::NA<Result>::na();
5 args = make_tuple( a1, a2 );
6 if ( mep )
7 signalWork();
8 return detail::NA<Result>::na();
9 }

1 virtual void complete() {
2 if ( !args )
3 return;
4 Args tmp_args = args.val();
5 f( get<0>(tmp_args), get<1>(tmp_args) );
6 args.clear();
7 }

This is from "EventProcessor.hpp" of RTT pre 2.0.
There are a few problems with it.
First of all line 4: I've added that. The args are read and stored in a
temporary variable. Then this is used to get the arguments for the function
call.
Before the args where read for each argument. You could end up with
inconsistent data the the subsequent read's where pre-empted by a call to
handler(). I should have fixed this as line 4 is a read out of a
DataObjectLockFree.

Now the second problem. If complete() is pre-empted by handle() between line
5 and 6, the newly stored args get cleared by line 6 (args.clear()). We
don't like this.
Any ideas on detecting this situation and counter measures?

Of course this will all be fixed in 2.0, but we're still not on that
version...

Kind regards,
Butch.

Fwd: Bug(s) in EventCatcher

Fixed this with the following patch. I hope it will be useful to someone.

Index: orocos/corelib/events/current/include/EventProcessor.hpp
===================================================================
--- orocos/corelib/events/current/include/EventProcessor.hpp (revision
3581)
+++ orocos/corelib/events/current/include/EventProcessor.hpp (working
copy)
@@ -191,22 +191,29 @@
template< class T>
struct Data
{
- Data() : work(false), val_("EventData") {}
- bool work;
+ Data() : tag(0), tagged(0), val_("EventData") {}
+ volatile int tag;
+ mutable int tagged;
DataObjectLockFree<T> val_;
typedef T type;
operator bool() const {
- return work;
+ /*Check for work.*/
+ return (tagged != tag);
}
void operator=(const T& t) {
val_.Set(t);
- work = true;
+ tag++;
}
T val() const {
+ tagged = tag;
return val_.Get();
}
void clear() {
- work = false;
+ if(OS::CAS(&tag, tagged, tagged+1))
+ {
+ /*Try to clear work. If pre-empted here by
operator=() tag will be incremented toO.*/
+ tagged++;
+ }
}
};
};
@@ -243,10 +250,11 @@
}

virtual void complete() {
- if ( !args )
- return;
- f( get<0>(args.val()), get<1>(args.val()) );
- args.clear();
+ if ( !args )
+ return;
+ Args tmp_args = args.val();
+ f( get<0>(tmp_args), get<1>(tmp_args) );
+ args.clear();
}
};

@@ -287,7 +295,8 @@
virtual void complete() {
if ( !args )
return;
- f( get<0>(args.val()), get<1>(args.val()),
get<2>(args.val()) );
+ Args tmp_args = args.val();
+ f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args) );
args.clear();
}
};
@@ -335,7 +344,8 @@
virtual void complete() {
if ( !args )
return;
- f( get<0>(args.val()), get<1>(args.val()),
get<2>(args.val()), get<3>(args.val()) );
+ Args tmp_args = args.val();
+ f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
get<3>(tmp_args) );
args.clear();
}
};
@@ -387,7 +397,8 @@
virtual void complete() {
if ( !args )
return;
- f( get<0>(args.val()), get<1>(args.val()),
get<2>(args.val()), get<3>(args.val()), get<4>(args.val()) );
+ Args tmp_args = args.val();
+ f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
get<3>(tmp_args), get<4>(tmp_args) );
args.clear();
}
};
@@ -437,7 +448,8 @@
virtual void complete() {
if ( !args )
return;
- f( get<0>(args.val()), get<1>(args.val()),
get<2>(args.val()), get<3>(args.val()), get<4>(args.val()),
get<5>(args.val()) );
+ Args tmp_args = args.val();
+ f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
get<3>(tmp_args), get<4>(tmp_args), get<5>(tmp_args) );
args.clear();
}
};

---------- Forwarded message ----------
From: Butch Slayer <butch [dot] slayers [..] ...>
Date: 2010/4/16
Subject: Bug(s) in EventCatcher
To: "orocos-dev [..] ..." <orocos-dev [..] ...>

Hi,

I need some advice on the following:

1 Result handler( typename Function::arg1_type a1,
2 typename Function::arg2_type a2 ) {
3 if ( !this->enabled )
4 return detail::NA<Result>::na();
5 args = make_tuple( a1, a2 );
6 if ( mep )
7 signalWork();
8 return detail::NA<Result>::na();
9 }

1 virtual void complete() {
2 if ( !args )
3 return;
4 Args tmp_args = args.val();
5 f( get<0>(tmp_args), get<1>(tmp_args) );
6 args.clear();
7 }

This is from "EventProcessor.hpp" of RTT pre 2.0.
There are a few problems with it.
First of all line 4: I've added that. The args are read and stored in a
temporary variable. Then this is used to get the arguments for the function
call.
Before the args where read for each argument. You could end up with
inconsistent data the the subsequent read's where pre-empted by a call to
handler(). I should have fixed this as line 4 is a read out of a
DataObjectLockFree.

Now the second problem. If complete() is pre-empted by handle() between line
5 and 6, the newly stored args get cleared by line 6 (args.clear()). We
don't like this.
Any ideas on detecting this situation and counter measures?

Of course this will all be fixed in 2.0, but we're still not on that
version...

Kind regards,
Butch.

Fwd: Bug(s) in EventCatcher

Dear Butch,

Thanks for the patch, I'm willing to apply it on the rtt trunk, but, your mail
reader crippled the patch seriously. Please consider sending an attachment ?

Peter

On Friday 23 April 2010 16:56:48 Butch Slayer wrote:
> Fixed this with the following patch. I hope it will be useful to someone.
>
> Index: orocos/corelib/events/current/include/EventProcessor.hpp
> ===================================================================
> --- orocos/corelib/events/current/include/EventProcessor.hpp (revision
> 3581)
> +++ orocos/corelib/events/current/include/EventProcessor.hpp (working
> copy)
> @@ -191,22 +191,29 @@
> template< class T>
> struct Data
> {
> - Data() : work(false), val_("EventData") {}
> - bool work;
> + Data() : tag(0), tagged(0), val_("EventData") {}
> + volatile int tag;
> + mutable int tagged;
> DataObjectLockFree<T> val_;
> typedef T type;
> operator bool() const {
> - return work;
> + /*Check for work.*/
> + return (tagged != tag);
> }
> void operator=(const T& t) {
> val_.Set(t);
> - work = true;
> + tag++;
> }
> T val() const {
> + tagged = tag;
> return val_.Get();
> }
> void clear() {
> - work = false;
> + if(OS::CAS(&tag, tagged, tagged+1))
> + {
> + /*Try to clear work. If pre-empted here by
> operator=() tag will be incremented toO.*/
> + tagged++;
> + }
> }
> };
> };
> @@ -243,10 +250,11 @@
> }
>
> virtual void complete() {
> - if ( !args )
> - return;
> - f( get<0>(args.val()), get<1>(args.val()) );
> - args.clear();
> + if ( !args )
> + return;
> + Args tmp_args = args.val();
> + f( get<0>(tmp_args), get<1>(tmp_args) );
> + args.clear();
> }
> };
>
> @@ -287,7 +295,8 @@
> virtual void complete() {
> if ( !args )
> return;
> - f( get<0>(args.val()), get<1>(args.val()),
> get<2>(args.val()) );
> + Args tmp_args = args.val();
> + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args) );
> args.clear();
> }
> };
> @@ -335,7 +344,8 @@
> virtual void complete() {
> if ( !args )
> return;
> - f( get<0>(args.val()), get<1>(args.val()),
> get<2>(args.val()), get<3>(args.val()) );
> + Args tmp_args = args.val();
> + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> get<3>(tmp_args) );
> args.clear();
> }
> };
> @@ -387,7 +397,8 @@
> virtual void complete() {
> if ( !args )
> return;
> - f( get<0>(args.val()), get<1>(args.val()),
> get<2>(args.val()), get<3>(args.val()), get<4>(args.val()) );
> + Args tmp_args = args.val();
> + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> get<3>(tmp_args), get<4>(tmp_args) );
> args.clear();
> }
> };
> @@ -437,7 +448,8 @@
> virtual void complete() {
> if ( !args )
> return;
> - f( get<0>(args.val()), get<1>(args.val()),
> get<2>(args.val()), get<3>(args.val()), get<4>(args.val()),
> get<5>(args.val()) );
> + Args tmp_args = args.val();
> + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> get<3>(tmp_args), get<4>(tmp_args), get<5>(tmp_args) );
> args.clear();
> }
> };
>
>
> ---------- Forwarded message ----------
> From: Butch Slayer <butch [dot] slayers [..] ...>
> Date: 2010/4/16
> Subject: Bug(s) in EventCatcher
> To: "orocos-dev [..] ..." <orocos-dev [..] ...>
>
>
> Hi,
>
> I need some advice on the following:
>
> 1 Result handler( typename Function::arg1_type a1,
> 2 typename Function::arg2_type a2 ) {
> 3 if ( !this->enabled )
> 4 return detail::NA<Result>::na();
> 5 args = make_tuple( a1, a2 );
> 6 if ( mep )
> 7 signalWork();
> 8 return detail::NA<Result>::na();
> 9 }
>
> 1 virtual void complete() {
> 2 if ( !args )
> 3 return;
> 4 Args tmp_args = args.val();
> 5 f( get<0>(tmp_args), get<1>(tmp_args) );
> 6 args.clear();
> 7 }
>
> This is from "EventProcessor.hpp" of RTT pre 2.0.
> There are a few problems with it.
> First of all line 4: I've added that. The args are read and stored in a
> temporary variable. Then this is used to get the arguments for the function
> call.
> Before the args where read for each argument. You could end up with
> inconsistent data the the subsequent read's where pre-empted by a call to
> handler(). I should have fixed this as line 4 is a read out of a
> DataObjectLockFree.
>
> Now the second problem. If complete() is pre-empted by handle() between
> line 5 and 6, the newly stored args get cleared by line 6 (args.clear()).
> We don't like this.
> Any ideas on detecting this situation and counter measures?
>
> Of course this will all be fixed in 2.0, but we're still not on that
> version...
>
> Kind regards,
> Butch.
>

Fwd: Bug(s) in EventCatcher

Peter,

I had added the patch as an attachment to. Please check the mail of 23 apr.
I'll send it again if you can't find it.
Are you planning on patching 1.10.x to?

Kind regards,
Butch.

2010/5/3 Peter Soetens <peter [dot] soetens [..] ...>

> Dear Butch,
>
> Thanks for the patch, I'm willing to apply it on the rtt trunk, but, your
> mail
> reader crippled the patch seriously. Please consider sending an attachment
> ?
>
> Peter
>
> On Friday 23 April 2010 16:56:48 Butch Slayer wrote:
> > Fixed this with the following patch. I hope it will be useful to someone.
> >
> > Index: orocos/corelib/events/current/include/EventProcessor.hpp
> > ===================================================================
> > --- orocos/corelib/events/current/include/EventProcessor.hpp (revision
> > 3581)
> > +++ orocos/corelib/events/current/include/EventProcessor.hpp (working
> > copy)
> > @@ -191,22 +191,29 @@
> > template< class T>
> > struct Data
> > {
> > - Data() : work(false), val_("EventData") {}
> > - bool work;
> > + Data() : tag(0), tagged(0), val_("EventData") {}
> > + volatile int tag;
> > + mutable int tagged;
> > DataObjectLockFree<T> val_;
> > typedef T type;
> > operator bool() const {
> > - return work;
> > + /*Check for work.*/
> > + return (tagged != tag);
> > }
> > void operator=(const T& t) {
> > val_.Set(t);
> > - work = true;
> > + tag++;
> > }
> > T val() const {
> > + tagged = tag;
> > return val_.Get();
> > }
> > void clear() {
> > - work = false;
> > + if(OS::CAS(&tag, tagged, tagged+1))
> > + {
> > + /*Try to clear work. If pre-empted here by
> > operator=() tag will be incremented toO.*/
> > + tagged++;
> > + }
> > }
> > };
> > };
> > @@ -243,10 +250,11 @@
> > }
> >
> > virtual void complete() {
> > - if ( !args )
> > - return;
> > - f( get<0>(args.val()), get<1>(args.val()) );
> > - args.clear();
> > + if ( !args )
> > + return;
> > + Args tmp_args = args.val();
> > + f( get<0>(tmp_args), get<1>(tmp_args) );
> > + args.clear();
> > }
> > };
> >
> > @@ -287,7 +295,8 @@
> > virtual void complete() {
> > if ( !args )
> > return;
> > - f( get<0>(args.val()), get<1>(args.val()),
> > get<2>(args.val()) );
> > + Args tmp_args = args.val();
> > + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args)
> );
> > args.clear();
> > }
> > };
> > @@ -335,7 +344,8 @@
> > virtual void complete() {
> > if ( !args )
> > return;
> > - f( get<0>(args.val()), get<1>(args.val()),
> > get<2>(args.val()), get<3>(args.val()) );
> > + Args tmp_args = args.val();
> > + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> > get<3>(tmp_args) );
> > args.clear();
> > }
> > };
> > @@ -387,7 +397,8 @@
> > virtual void complete() {
> > if ( !args )
> > return;
> > - f( get<0>(args.val()), get<1>(args.val()),
> > get<2>(args.val()), get<3>(args.val()), get<4>(args.val()) );
> > + Args tmp_args = args.val();
> > + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> > get<3>(tmp_args), get<4>(tmp_args) );
> > args.clear();
> > }
> > };
> > @@ -437,7 +448,8 @@
> > virtual void complete() {
> > if ( !args )
> > return;
> > - f( get<0>(args.val()), get<1>(args.val()),
> > get<2>(args.val()), get<3>(args.val()), get<4>(args.val()),
> > get<5>(args.val()) );
> > + Args tmp_args = args.val();
> > + f( get<0>(tmp_args), get<1>(tmp_args), get<2>(tmp_args),
> > get<3>(tmp_args), get<4>(tmp_args), get<5>(tmp_args) );
> > args.clear();
> > }
> > };
> >
> >
> > ---------- Forwarded message ----------
> > From: Butch Slayer <butch [dot] slayers [..] ...>
> > Date: 2010/4/16
> > Subject: Bug(s) in EventCatcher
> > To: "orocos-dev [..] ..." <
> orocos-dev [..] ...>
> >
> >
> > Hi,
> >
> > I need some advice on the following:
> >
> > 1 Result handler( typename Function::arg1_type a1,
> > 2 typename Function::arg2_type a2 ) {
> > 3 if ( !this->enabled )
> > 4 return detail::NA<Result>::na();
> > 5 args = make_tuple( a1, a2 );
> > 6 if ( mep )
> > 7 signalWork();
> > 8 return detail::NA<Result>::na();
> > 9 }
> >
> > 1 virtual void complete() {
> > 2 if ( !args )
> > 3 return;
> > 4 Args tmp_args = args.val();
> > 5 f( get<0>(tmp_args), get<1>(tmp_args) );
> > 6 args.clear();
> > 7 }
> >
> > This is from "EventProcessor.hpp" of RTT pre 2.0.
> > There are a few problems with it.
> > First of all line 4: I've added that. The args are read and stored in a
> > temporary variable. Then this is used to get the arguments for the
> function
> > call.
> > Before the args where read for each argument. You could end up with
> > inconsistent data the the subsequent read's where pre-empted by a call to
> > handler(). I should have fixed this as line 4 is a read out of a
> > DataObjectLockFree.
> >
> > Now the second problem. If complete() is pre-empted by handle() between
> > line 5 and 6, the newly stored args get cleared by line 6
> (args.clear()).
> > We don't like this.
> > Any ideas on detecting this situation and counter measures?
> >
> > Of course this will all be fixed in 2.0, but we're still not on that
> > version...
> >
> > Kind regards,
> > Butch.
> >
>

Fwd: Bug(s) in EventCatcher

On Tuesday 04 May 2010 13:56:49 Butch Slayer wrote:
> Peter,
>
> I had added the patch as an attachment to. Please check the mail of 23 apr.
> I'll send it again if you can't find it.
> Are you planning on patching 1.10.x to?

Yes. I could apply your patch on 1.10. The 1.10 branch also contains code for
'signalWork()' and 'signalWorkDone()', which is used to keep track of number
of events to be processed. I don't think this interferes with your code, since
its only a counter used by hasWork() to report if the value of the counter.
hasWork is only used by the SequentialActivity implementation.

Thanks for sharing this.

Peter

Fwd: Bug(s) in EventCatcher

No problem, this is how the system works!

2010/6/18 Peter Soetens <peter [..] ...>

> On Tuesday 04 May 2010 13:56:49 Butch Slayer wrote:
> > Peter,
> >
> > I had added the patch as an attachment to. Please check the mail of 23
> apr.
> > I'll send it again if you can't find it.
> > Are you planning on patching 1.10.x to?
>
> Yes. I could apply your patch on 1.10. The 1.10 branch also contains code
> for
> 'signalWork()' and 'signalWorkDone()', which is used to keep track of
> number
> of events to be processed. I don't think this interferes with your code,
> since
> its only a counter used by hasWork() to report if the value of the counter.
> hasWork is only used by the SequentialActivity implementation.
>
> Thanks for sharing this.
>
> Peter
>