Re: QuantLib-SWIG, the observer pattern and destructor call during update

[Luigi Ballabio]

Hi all,
    recently Klaus Spanderen has posted on his blog about the
Observer/Observable problems we had when QuantLib is exported through
SWIG to a language like Java or C# which run garbage collection in a
separate thread (see
<http://old.nabble.com/Issues-with-C--Swig-Bindings,-NUnit-and-Settings.instance%28%29.setEvaluationDate%28%29-td30549787.html>
for the original thread and
<http://hpcquantlib.wordpress.com/2012/02/27/quantlib-swig-and-a-thread-safe-observer-pattern-in-c/>
for Klaus' post).

I've been thinking about it for the last few months, too.  I haven't a
full solution (I guess the real solution would be to rewrite the whole
thing in terms of Boost.Signal2, which is thread-safe) but here's what
I got so far.

The original problem is that we've gone against one of the basic
tenets of C++, namely, that release of resources should go in the
inverse order as their acquisition. What happens now is that observers
unregister themselves in the destructor they inherit from the base
Observer class, which results in the following sequence of actions:

On construction:
- call the Observer constructor, which builds the base-class part of
the instance;
- call the derived-class constructor, which builds the derived-class
attributes and stuff;
- inside the derived-class constructor, register with the observables.

On destruction:
- call the derived-class destructor, which destroys the derived-class
attributes and stuff;
- call the Observer destructor, which unregisters with the observables...
- ...and then destroys the base-class part of the instance.

The problem is that we have a-b-c during construction and b-c-a during
destruction (it should be c-b-a).  Doing it this way, sometimes it
happens that an observable sends a notification between b and c. The
observer is not yet unregistered, so it gets the notification; but
since it's already been partially destroyed, the resulting call to
update() results in a crash.

Both Henner and Klaus did their best to fix the Observer class, but I
think the solution is to do things in the correct order (c-b-a), that
is, unregister in the destructor of the derived class.  But how?
Forcing one to write the calls to unregisterWith() inside the
destructor (and often, to write an explicit destructor just for that)
cannot be enforced, and would result in dangling pointers as soon as
one forgets to do it.

One way might be to use RAII to do this.  We might implement a helper
class like:

template <class T>
class Registered {
    T observable_;
    Observer* observer_;
  public:
    Registered(const T& observable, Observer* observer)
    : observable_(observable), observer_(observer) {
        observer_->registerWith(observable_);
    }
    ~Registered() {
        observer_->unregisterWith(observable_);
    }
    const T& operator->() const { return observable_; }
};

that wraps an observable and manages unregistration.  This way,
instead of writing derived observer classes as:

class SomeClass {
    Handle<YieldTermStructure> ts_;
  public:
    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts) {
        registerWith(ts_);
    }
};

we would write:

class SomeClass {
    Registered<Handle<YieldTermStructure> > ts_;
  public:
    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts, this) {}
};

This way, ts_ is a Registered instance (which provides an
operator->(), so it can be used as before; for instance,
ts_->discount(t) still works) and when SomeClass is destroyed, the
unregisterWith call in the Registered destructor will fire during the
destruction of SomeClass, doing things in the correct order.

(Note: it would also be possible to work out things so that we can
leave the registerWith() call in the constructor, if we want to modify
the least possible amount of code. When called with a Registered as an
argument, it would register the observer with the wrapped observable
and store the observer's "this" pointer into the Registered instance.)
(Note 2: "Registered" might not be the best name.  "Observed", maybe?)


Unfortunately, it's not foolproof.  For instance, the Registered
instances are better declared last in the class; and even in that
case, if we have two Registered (A and B) there might be freak
scenarios in which A is unregistered and destroyed, and before B can
be unregistered it fires a notification.  If the observer's update()
method just flips a bool, it will work fine; but if update() tries to
access A instead, it will find it destroyed and hilarity will ensue.

Also, we would still have the problem that an observer might be
removed from an observable's registered list while the observable is
iterating over it in notifyObservers().  We'll need a lock to prevent
that.

I guess this about wraps it up.  Thoughts?

Later,
    Luigi

------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
QuantLib-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quantlib-dev

[Ferdinando M. Ametrano-3]

Hi all

I wonder if it would be more effective to just live with the fact that
QuantLib 1.x is not thread-safe and start a parallel
non-backward-compatible QuantLib 2.x ?

We might use 2.x for an overall library re-factoring, with the spotlight on
1) thread-safety
2) parallelism
3) increased boost-ification (math, distributions, date, random
numbers, optional, etc)
4) review of few issues about Instrument/Pricing engines, etc
5) interface clean-up removing methods which can be implemented as functions
6) review of the worst performance bottlenecks (e.g. virtual
Event::hasOccurred, etc)
7) fix the non invertible relation between dates and times in TermStructure
8) etc

ciao -- Nando

On Wed, Feb 29, 2012 at 12:45 PM, Luigi Ballabio
<[hidden email]> wrote:

> Hi all,
>    recently Klaus Spanderen has posted on his blog about the
> Observer/Observable problems we had when QuantLib is exported through
> SWIG to a language like Java or C# which run garbage collection in a
> separate thread (see
> <http://old.nabble.com/Issues-with-C--Swig-Bindings,-NUnit-and-Settings.instance%28%29.setEvaluationDate%28%29-td30549787.html>
> for the original thread and
> <http://hpcquantlib.wordpress.com/2012/02/27/quantlib-swig-and-a-thread-safe-observer-pattern-in-c/>
> for Klaus' post).
>
> I've been thinking about it for the last few months, too.  I haven't a
> full solution (I guess the real solution would be to rewrite the whole
> thing in terms of Boost.Signal2, which is thread-safe) but here's what
> I got so far.
>
> The original problem is that we've gone against one of the basic
> tenets of C++, namely, that release of resources should go in the
> inverse order as their acquisition. What happens now is that observers
> unregister themselves in the destructor they inherit from the base
> Observer class, which results in the following sequence of actions:
>
> On construction:
> - call the Observer constructor, which builds the base-class part of
> the instance;
> - call the derived-class constructor, which builds the derived-class
> attributes and stuff;
> - inside the derived-class constructor, register with the observables.
>
> On destruction:
> - call the derived-class destructor, which destroys the derived-class
> attributes and stuff;
> - call the Observer destructor, which unregisters with the observables...
> - ...and then destroys the base-class part of the instance.
>
> The problem is that we have a-b-c during construction and b-c-a during
> destruction (it should be c-b-a).  Doing it this way, sometimes it
> happens that an observable sends a notification between b and c. The
> observer is not yet unregistered, so it gets the notification; but
> since it's already been partially destroyed, the resulting call to
> update() results in a crash.
>
> Both Henner and Klaus did their best to fix the Observer class, but I
> think the solution is to do things in the correct order (c-b-a), that
> is, unregister in the destructor of the derived class.  But how?
> Forcing one to write the calls to unregisterWith() inside the
> destructor (and often, to write an explicit destructor just for that)
> cannot be enforced, and would result in dangling pointers as soon as
> one forgets to do it.
>
> One way might be to use RAII to do this.  We might implement a helper
> class like:
>
> template <class T>
> class Registered {
>    T observable_;
>    Observer* observer_;
>  public:
>    Registered(const T& observable, Observer* observer)
>    : observable_(observable), observer_(observer) {
>        observer_->registerWith(observable_);
>    }
>    ~Registered() {
>        observer_->unregisterWith(observable_);
>    }
>    const T& operator->() const { return observable_; }
> };
>
> that wraps an observable and manages unregistration.  This way,
> instead of writing derived observer classes as:
>
> class SomeClass {
>    Handle<YieldTermStructure> ts_;
>  public:
>    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts) {
>        registerWith(ts_);
>    }
> };
>
> we would write:
>
> class SomeClass {
>    Registered<Handle<YieldTermStructure> > ts_;
>  public:
>    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts, this) {}
> };
>
> This way, ts_ is a Registered instance (which provides an
> operator->(), so it can be used as before; for instance,
> ts_->discount(t) still works) and when SomeClass is destroyed, the
> unregisterWith call in the Registered destructor will fire during the
> destruction of SomeClass, doing things in the correct order.
>
> (Note: it would also be possible to work out things so that we can
> leave the registerWith() call in the constructor, if we want to modify
> the least possible amount of code. When called with a Registered as an
> argument, it would register the observer with the wrapped observable
> and store the observer's "this" pointer into the Registered instance.)
> (Note 2: "Registered" might not be the best name.  "Observed", maybe?)
>
>
> Unfortunately, it's not foolproof.  For instance, the Registered
> instances are better declared last in the class; and even in that
> case, if we have two Registered (A and B) there might be freak
> scenarios in which A is unregistered and destroyed, and before B can
> be unregistered it fires a notification.  If the observer's update()
> method just flips a bool, it will work fine; but if update() tries to
> access A instead, it will find it destroyed and hilarity will ensue.
>
> Also, we would still have the problem that an observer might be
> removed from an observable's registered list while the observable is
> iterating over it in notifyObservers().  We'll need a lock to prevent
> that.
>
> I guess this about wraps it up.  Thoughts?
>
> Later,
>    Luigi
>
> ------------------------------------------------------------------------------
> Virtualization & Cloud Management Using Capacity Planning
> Cloud computing makes use of virtualization - but cloud computing
> also focuses on allowing computing to be delivered as a service.
> http://www.accelacomm.com/jaw/sfnl/114/51521223/
> _______________________________________________
> QuantLib-dev mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/quantlib-dev

------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
QuantLib-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quantlib-dev

[Luigi Ballabio]

On Wed, Feb 29, 2012 at 4:51 PM, Ferdinando Ametrano <[hidden email]> wrote:
> I wonder if it would be more effective to just live with the fact that
> QuantLib 1.x is not thread-safe and start a parallel
> non-backward-compatible QuantLib 2.x ?

That's a possibility (and seeing the number of issues we'll have to
address for 2.0, we'll also have to discuss whether to keep using
Subversion or move to a DVCS).

However, the problem is that even if one doesn't use threads, as soon
as one uses QuantLib in Java or C# the system is going to run the
garbage collector in another thread.
So until a 2.0 is out, version 1.x is pretty much unusable from those
languages for anything else than toy programs.

Luigi

> We might use 2.x for an overall library re-factoring, with the spotlight on
> 1) thread-safety
> 2) parallelism
> 3) increased boost-ification (math, distributions, date, random
> numbers, optional, etc)
> 4) review of few issues about Instrument/Pricing engines, etc
> 5) interface clean-up removing methods which can be implemented as functions
> 6) review of the worst performance bottlenecks (e.g. virtual
> Event::hasOccurred, etc)
> 7) fix the non invertible relation between dates and times in TermStructure
> 8) etc
>
> ciao -- Nando
>
> On Wed, Feb 29, 2012 at 12:45 PM, Luigi Ballabio
> <[hidden email]> wrote:
>> Hi all,
>>    recently Klaus Spanderen has posted on his blog about the
>> Observer/Observable problems we had when QuantLib is exported through
>> SWIG to a language like Java or C# which run garbage collection in a
>> separate thread (see
>> <http://old.nabble.com/Issues-with-C--Swig-Bindings,-NUnit-and-Settings.instance%28%29.setEvaluationDate%28%29-td30549787.html>
>> for the original thread and
>> <http://hpcquantlib.wordpress.com/2012/02/27/quantlib-swig-and-a-thread-safe-observer-pattern-in-c/>
>> for Klaus' post).
>>
>> I've been thinking about it for the last few months, too.  I haven't a
>> full solution (I guess the real solution would be to rewrite the whole
>> thing in terms of Boost.Signal2, which is thread-safe) but here's what
>> I got so far.
>>
>> The original problem is that we've gone against one of the basic
>> tenets of C++, namely, that release of resources should go in the
>> inverse order as their acquisition. What happens now is that observers
>> unregister themselves in the destructor they inherit from the base
>> Observer class, which results in the following sequence of actions:
>>
>> On construction:
>> - call the Observer constructor, which builds the base-class part of
>> the instance;
>> - call the derived-class constructor, which builds the derived-class
>> attributes and stuff;
>> - inside the derived-class constructor, register with the observables.
>>
>> On destruction:
>> - call the derived-class destructor, which destroys the derived-class
>> attributes and stuff;
>> - call the Observer destructor, which unregisters with the observables...
>> - ...and then destroys the base-class part of the instance.
>>
>> The problem is that we have a-b-c during construction and b-c-a during
>> destruction (it should be c-b-a).  Doing it this way, sometimes it
>> happens that an observable sends a notification between b and c. The
>> observer is not yet unregistered, so it gets the notification; but
>> since it's already been partially destroyed, the resulting call to
>> update() results in a crash.
>>
>> Both Henner and Klaus did their best to fix the Observer class, but I
>> think the solution is to do things in the correct order (c-b-a), that
>> is, unregister in the destructor of the derived class.  But how?
>> Forcing one to write the calls to unregisterWith() inside the
>> destructor (and often, to write an explicit destructor just for that)
>> cannot be enforced, and would result in dangling pointers as soon as
>> one forgets to do it.
>>
>> One way might be to use RAII to do this.  We might implement a helper
>> class like:
>>
>> template <class T>
>> class Registered {
>>    T observable_;
>>    Observer* observer_;
>>  public:
>>    Registered(const T& observable, Observer* observer)
>>    : observable_(observable), observer_(observer) {
>>        observer_->registerWith(observable_);
>>    }
>>    ~Registered() {
>>        observer_->unregisterWith(observable_);
>>    }
>>    const T& operator->() const { return observable_; }
>> };
>>
>> that wraps an observable and manages unregistration.  This way,
>> instead of writing derived observer classes as:
>>
>> class SomeClass {
>>    Handle<YieldTermStructure> ts_;
>>  public:
>>    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts) {
>>        registerWith(ts_);
>>    }
>> };
>>
>> we would write:
>>
>> class SomeClass {
>>    Registered<Handle<YieldTermStructure> > ts_;
>>  public:
>>    SomeClass(const Handle<YieldTermStructure>& ts) : ts_(ts, this) {}
>> };
>>
>> This way, ts_ is a Registered instance (which provides an
>> operator->(), so it can be used as before; for instance,
>> ts_->discount(t) still works) and when SomeClass is destroyed, the
>> unregisterWith call in the Registered destructor will fire during the
>> destruction of SomeClass, doing things in the correct order.
>>
>> (Note: it would also be possible to work out things so that we can
>> leave the registerWith() call in the constructor, if we want to modify
>> the least possible amount of code. When called with a Registered as an
>> argument, it would register the observer with the wrapped observable
>> and store the observer's "this" pointer into the Registered instance.)
>> (Note 2: "Registered" might not be the best name.  "Observed", maybe?)
>>
>>
>> Unfortunately, it's not foolproof.  For instance, the Registered
>> instances are better declared last in the class; and even in that
>> case, if we have two Registered (A and B) there might be freak
>> scenarios in which A is unregistered and destroyed, and before B can
>> be unregistered it fires a notification.  If the observer's update()
>> method just flips a bool, it will work fine; but if update() tries to
>> access A instead, it will find it destroyed and hilarity will ensue.
>>
>> Also, we would still have the problem that an observer might be
>> removed from an observable's registered list while the observable is
>> iterating over it in notifyObservers().  We'll need a lock to prevent
>> that.
>>
>> I guess this about wraps it up.  Thoughts?
>>
>> Later,
>>    Luigi
>>
>> ------------------------------------------------------------------------------
>> Virtualization & Cloud Management Using Capacity Planning
>> Cloud computing makes use of virtualization - but cloud computing
>> also focuses on allowing computing to be delivered as a service.
>> http://www.accelacomm.com/jaw/sfnl/114/51521223/
>> _______________________________________________
>> QuantLib-dev mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/quantlib-dev

------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
QuantLib-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quantlib-dev

[Bojan Nikolic]

Hi Luigi,

I haven't fully worked this one out, but I think that it is possible to
some progress on this by making the destructor of the Observer class
private and then creating some helper classes (probably inheriting from
shared_pointer) that are friends of Observer and define proper clean
order, i.e., disconnect the Observables before starting the main
destruction.

This approach would enforce code correctness at compile time (because
non-safe destruction of Observer objects would be a compile time error)
but would obviously require changes to existing code. It should play
fairly well with SWIG which is I think already aware of the implications
of a private destructor.

There is no less invasive way I can think of at moment -- the goal of
having easy composition of objects in C++ makes it very hard to re-order
destruction operations! Possibly one can do something very clever with
clone()-able objects and overriding the operator new() for Observer but
that would be getting really complicated....

Best,
Bojan

--
Bojan Nikolic          ||          http://www.bnikolic.co.uk

------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
QuantLib-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quantlib-dev

[Bojan Nikolic]

Bojan Nikolic <[hidden email]> writes:

> I haven't fully worked this one out, but I think that it is possible to
> some progress on this by making the destructor of the Observer class
> private and then creating some helper classes (probably inheriting from
> shared_pointer) that are friends of Observer and define proper clean
> order, i.e., disconnect the Observables before starting the main
> destruction.

I thought some more about this and realised that private/protected
constructors nor overloading operator new would really help. As far as I
can see the only way to fully solve this type of problem in C++ is to
have control over the storage/object lifetime of Observer objects which
would mean restricting their creation to within object factories only.

This would be a big change for the C++ code base. However, the nice
thing is that SWIG, when creating the interface layer, essentially does
this control of object storage. So, by suitable use of SWIG directives I
think it is in fact possible to have a completely clean destruction
procedure. Something like:

%typemap(javadestruct,
         methodname="delete",
         methodmodifiers="public synchronized")  
{
        // Replace Observable with correct QuantLib/Java name
        if (this instanceof Observable) {
        // Translate to Java
        for (iterator i=observables_.begin(); i!=observables_.end(); ++i)
            (*i)->unregisterObserver(this);            
        }
       
      // This is the original javafdestruct code
      if (swigCPtr != 0) {
      if (swigCMemOwn) {
        swigCMemOwn = false;
        $jnicall;
      }
      swigCPtr = 0;
    }
}

should do the trick nicely. Sorry, haven't got around to trying this
yet.

Best,
Bojan

--
Bojan Nikolic          ||          http://www.bnikolic.co.uk

------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
QuantLib-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quantlib-dev