This must be a regression - at least the fortuneclients all used to work, IIRC.

Prioritizing P2.

*** Bug 521 has been marked as a duplicate of this bug. ***

fixed on shiboken commit:

commit fb4035a69475759e58e25d5aa0f061d6c212f4af
Author: Renato Araujo Oliveira Filho <renato.filho@openbossa.org>
Date:   Tue Dec 14 15:19:37 2010 -0300

Closign bugs after release of 1.0.0 beta2.

On Windows 1.0.0-beta3, this is still not fixed.

OK, retaining P2 prioritization until the issue has been re-investigated.

It works on Linux, we are going to check it on Windows and add a unit test.

The __del__ still get called before the C++ object get destroyed, but...

Before the destruction of Q(Core)Application we need to destroy all QObjects to
avoid dead locks beside other issues... when fixing bug 552 I saw that we were
destroying the QObjects but not setting the object as invalid, so when the
__del__ gets called by the Python interpreter if the python object access the
C++ object a crash could happen because the Python object didn't get the notice
that the C++ object was deleted.

I fixed this in a local commit as it is the fix for bug 552 and now it sets the
object as invalid and you get the runtime exception, but...

Conclusion: When inside the __del__ method you can have access to the C++
object or not, it depends if the QApplication was destroyed or not and if your
class is inherited from QObject.

I can easilly fix the examples by connecting using the destroyed signal, but I
guess we should open another bug to document this __del__ behaviour somewhere.

(In reply to comment #8)
> Before the destruction of Q(Core)Application we need to destroy all QObjects to
> avoid dead locks beside other issues...

I assume this is a PySide-only behavior? If so, is there something that
prevents you from deleting objects at the Shiboken/Python level rather than
doing it only at the Qt/C++ level? And if you can't delete them, wouldn't it
still be possible to call their __del__ method and replace the whole object by
a placeholder that raises an exception whenever any method is called? (that is,
it would be like the solution you're describing except it would include Python
code instead of stopping at the C++ boundary)

> Yes it works fine.
> 
> I don't really like removing the mutex as it relies on the existence of a GIL
> (CPython itself hopes to remove it at some point). A simple renaming of __del__
> could do the job just as well:

You are right, it's even simpler.

> (In reply to comment #10)
> > The problem with the Python code is that it rely on the object destruction
> > order, but Python don't have a deterministic one as C++ have, this is why the
> > C++ code works without warning and the Python one not.
> 
> Not really: I don't think Python would delete a base class instance before it
> deletes the derived class instance. So it should be possible for __del__ to be
> called with the guarantee that the QThread hasn't been destroyed.

There's no two instances, no matter how many base classes exists there will be
just one PyObject and this PyObject need to have info about the C++ Object(s)
it represents so when the object dies it dies.

What about comment #9?

(In reply to comment #9)
> (In reply to comment #8)
> > Before the destruction of Q(Core)Application we need to destroy all QObjects to
> > avoid dead locks beside other issues...
> 
> I assume this is a PySide-only behavior? If so, is there something that
> prevents you from deleting objects at the Shiboken/Python level rather than
> doing it only at the Qt/C++ level? And if you can't delete them, wouldn't it
> still be possible to call their __del__ method and replace the whole object by
> a placeholder that raises an exception whenever any method is called? (that is,
> it would be like the solution you're describing except it would include Python
> code instead of stopping at the C++ boundary)

The problem is:

We overwrite QCoreApplication::notify method because it's virtual and need to
be overwritten to add the possibility to the user to reimplement it in Python.

When the Python interpreter dies, you can't call any Python C API function, and
our version of the notify method do.

When any QObject dies, if there's a QApplication it will call the notify method
to tell about your death.

This problem isn't limited to the notify method, but for any virtual method
that could be called at destruction time of any class.

We need to make sure that QApplication will be the last QObject to die, to
prove the problem try this C++ example:

#include <QtCore>
#include <QtGui>

int main(int argc, char** argv) {
    QApplication* app = new QApplication(argc, argv);
    QWidget* w = new QWidget;
    w->show();
    app->exec();
    delete app;
    delete w;
}


The solution? We need to make sure that QApplication will be the last one to
die, how!? Killing all other QObjects alive.

When we kill a QObject, we don't kill their Python reference (because we
can't!), we just delete the underlying C++ object and set the object as
invalid, so why not calling the __del__ at this moment!?

Because if we call the __del__ by ourselves, when the last reference to this
Python object vanishes the Python interpreter will call the __del__ method
again and for sure this is not a desired effect.

Why PyQt works? because it doesn't call the __del__ method and probably does
not even delete the QApplication to avoid this kind of problems. IMO this isn't
an option, so we took the decision of avoid this memory leak and suffer the
consequences

(In reply to comment #9)
> (In reply to comment #8)
> > Before the destruction of Q(Core)Application we need to destroy all QObjects to
> > avoid dead locks beside other issues...
> 
> I assume this is a PySide-only behavior? If so, is there something that
> prevents you from deleting objects at the Shiboken/Python level rather than
> doing it only at the Qt/C++ level? And if you can't delete them, wouldn't it
> still be possible to call their __del__ method and replace the whole object by
> a placeholder that raises an exception whenever any method is called? (that is,
> it would be like the solution you're describing except it would include Python
> code instead of stopping at the C++ boundary)

Calling the __del__ method when the objects still having a lot of references
sounds to me like a rape to Python reference model, but you could argue that in
pratice it's not too much different of setting a object as invalid because in
both cases the object will be useless for a period of time.

(In reply to comment #12)
> int main(int argc, char** argv) {
>     QApplication* app = new QApplication(argc, argv);
>     QWidget* w = new QWidget;
>     w->show();
>     app->exec();
>     delete app;
>     delete w;
> }

The code works on my system, and I don't see any problem with it: you have to
construct the QWidget after the QApplication but there's no reason you need to
delete it before deleting the QApplication. The code of ~QWidget takes steps to
handle this case, so it seems logical that it should be supported. Does it say
anywhere in the Qt doc that this is a bad idea?


Regarding PySide destruction: I'm not sure what exactly is happening. You're
saying that you're cleaning up when the interpreter is exiting, and that at
that point no Python code can run. Fine, but what happens to QThreads that are
running? Are they still running Python code, or are they suspended? If they're
still running, is the Python code executed successfully or will it crash?

If interpreter death is an issue, would it be feasible to move forward the
destruction of objects so that it happens when the interpreter is still alive?

Also, why is it so important to destroy objects when the process is exiting,
apart from the fact that it sounds cleaner? Are there platforms where the OS
can't clean up everything that Qt has allocated?

I understand your final point: forcefully destroying objects in a way that's
not transparent is highly unexpected in a scripting language, but if we're
really forced to do it then there's no harm in letting Python code know that
(so it can better deal with this forceful destruction). However it might not be
easy to implement, because we don't want to just destroy objects in a random
order...

(In reply to comment #14)
> (In reply to comment #12)
> > int main(int argc, char** argv) {
> >     QApplication* app = new QApplication(argc, argv);
> >     QWidget* w = new QWidget;
> >     w->show();
> >     app->exec();
> >     delete app;
> >     delete w;
> > }
> 
> The code works on my system, and I don't see any problem with it: you have to
> construct the QWidget after the QApplication but there's no reason you need to
> delete it before deleting the QApplication. The code of ~QWidget takes steps to
> handle this case, so it seems logical that it should be supported. Does it say
> anywhere in the Qt doc that this is a bad idea?

If it works for you it was a matter of coincidence, because with me if
segfaults 100% of times.

[hugo@thinkpadugo foo  4.7] $ cat > main.cpp                                    
#include <QtCore>                                                               
#include <QtGui>                                                                

int main(int argc, char** argv) {                                               
    QApplication* app = new QApplication(argc, argv);                           
    QWidget* w = new QWidget;                                                   
    w->show();                                                                  
    app->exec();                                                                
    delete app;                                                                 
    delete w;                                                                   
}                                                                               

[hugo@thinkpadugo foo  4.7] $ qmake -project                                    
[hugo@thinkpadugo foo  4.7] $ qmake                                             
[hugo@thinkpadugo foo  4.7] $ make                                              
g++ -c -m64 -pipe -march=x86-64 -mtune=generic -O2 -pipe -Wall -W -D_REENTRANT
-DQT_NO_DEBUG -DQT_GUI_LIB -DQT_CORE_LIB -DQT_SHARED
-I/usr/share/qt/mkspecs/linux-g++-64 -I. -I/usr/include/QtCore
-I/usr/include/QtGui -I/usr/include -I. -I. -o main.o main.cpp                  
g++ -m64 -Wl,--hash-style=gnu -Wl,--as-needed -Wl,-O1 -o foo main.o   
-L/usr/lib -lQtGui -lQtCore -lpthread                                           
[hugo@thinkpadugo foo  4.7] $ ./foo                                             
Segmentation fault                                                              
[hugo@thinkpadugo foo  4.7] $ gdb ./foo                                         
GNU gdb (GDB) 7.2                                                               
Copyright (C) 2010 Free Software Foundation, Inc.                               
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>   
This is free software: you are free to change and redistribute it.              
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"      
and "show warranty" for details.                                                
This GDB was configured as "x86_64-unknown-linux-gnu".                          
For bug reporting instructions, please see:                                     
<http://www.gnu.org/software/gdb/bugs/>...                                      
Reading symbols from /tmp/foo/foo...(no debugging symbols found)...done.        
(gdb) r                                                                         
Starting program: /tmp/foo/foo                                                  
[Thread debugging using libthread_db enabled]                                   
Qt: gdb: -nograb added to command-line options.                                 
         Use the -dograb option to enforce grabbing.                            

Program received signal SIGSEGV, Segmentation fault.                            
0x00007ffff73096d1 in QX11WindowSurface::~QX11WindowSurface (this=0x64f050,
__in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/painting/qwindowsurface_x11.cpp:84                    
84              XFreeGC(X11->display, gc);                                      
(gdb) bt                                                                        
#0  0x00007ffff73096d1 in QX11WindowSurface::~QX11WindowSurface (this=0x64f050,
__in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/painting/qwindowsurface_x11.cpp:84                    
#1  0x00007ffff730974c in QX11WindowSurface::~QX11WindowSurface (this=0x64f050,
__in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/painting/qwindowsurface_x11.cpp:87                    
#2  0x00007ffff7303203 in QWidgetBackingStore::~QWidgetBackingStore
(this=0x79a960, __in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/painting/qbackingstore.cpp:883                        
#3  0x00007ffff70be473 in QWidgetBackingStoreTracker::destroy (this=0x793260)
at /home/hugo/src/qt/src/gui/kernel/qwidget.cpp:205                             
#4  0x00007ffff70c15d2 in QWidgetPrivate::deleteExtra (this=0x673ff0) at
/home/hugo/src/qt/src/gui/kernel/qwidget.cpp:1750                               
#5  0x00007ffff70beb0c in QWidgetPrivate::~QWidgetPrivate (this=0x673ff0,
__in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/kernel/qwidget.cpp:318                                
#6  0x00007ffff70bed94 in QWidgetPrivate::~QWidgetPrivate (this=0x673ff0,
__in_chrg=<value optimized out>) at
/home/hugo/src/qt/src/gui/kernel/qwidget.cpp:323                                
#7  0x00007ffff6251b58 in QScopedPointerDeleter<QObjectData>::cleanup
(pointer=0x673ff0) at
../../include/QtCore/../../../qt/src/corelib/tools/qscopedpointer.h:62          
#8  0x00007ffff62505e9 in QScopedPointer<QObjectData,
QScopedPointerDeleter<QObjectData> >::~QScopedPointer (this=0x604658,
__in_chrg=<value optimized out>)                                                
    at ../../include/QtCore/../../../qt/src/corelib/tools/qscopedpointer.h:100  
#9  0x00007ffff6249825 in QObject::~QObject (this=0x604650, __in_chrg=<value
optimized out>) at /home/hugo/src/qt/src/corelib/kernel/qobject.cpp:817         
#10 0x00007ffff70c0e8d in QWidget::~QWidget (this=0x604650, __in_chrg=<value
optimized out>) at /home/hugo/src/qt/src/gui/kernel/qwidget.cpp:1477            
#11 0x00007ffff70c0fee in QWidget::~QWidget (this=0x604650, __in_chrg=<value
optimized out>) at /home/hugo/src/qt/src/gui/kernel/qwidget.cpp:1620            
#12 0x00000000004008de in main ()                                               

No matter if I use Qt in debug mode or the Qt in release mode used by my own
system.

> Regarding PySide destruction: I'm not sure what exactly is happening. You're
> saying that you're cleaning up when the interpreter is exiting, and that at
> that point no Python code can run. Fine, but what happens to QThreads that are
> running? Are they still running Python code, or are they suspended? If they're
> still running, is the Python code executed successfully or will it crash?

No, they will die because we kill all QObjects, anyway if the program finishes
and not all your threads get finished you have a logic problem on your code.

> If interpreter death is an issue, would it be feasible to move forward the
> destruction of objects so that it happens when the interpreter is still alive?
> 
> Also, why is it so important to destroy objects when the process is exiting,
> apart from the fact that it sounds cleaner? Are there platforms where the OS
> can't clean up everything that Qt has allocated?

That's no just about memory, but resources in general, like flushing contents
to files, etc...  many of these tasks are done on destructors when programming
in C++. Clean exit is crucial for any well formed program.

> I understand your final point: forcefully destroying objects in a way that's
> not transparent is highly unexpected in a scripting language, but if we're
> really forced to do it then there's no harm in letting Python code know that
> (so it can better deal with this forceful destruction). However it might not be
> easy to implement, because we don't want to just destroy objects in a random
> order...

In PySide we have more reliable alternatives to the __del__ method like the
destroyed signal, so I guess that what we should do is to document why you can
have problems writing your own __del__ function.

(In reply to comment #14)
> If interpreter death is an issue, would it be feasible to move forward the
> destruction of objects so that it happens when the interpreter is still alive?

Oops, I forgot this one.

We already do it, we destroy the (C++) objects when the interpreter still
alive, but we can't decref all Python references to destroy the PyObjects as
well. If we do so the Python interpreter probably will crash because we can
only decref references that we own... this is the law in Python C API.

So... Can I re-close this bug?

(In reply to comment #17)
> So... Can I re-close this bug?

If you think the issue is resolved, of course.

I think it's, and I think Farsno understood the whole picture of the __del__
method in Qt bindings, so re-closing it!

Sorry, didn't have time to respond any sooner.

Your answers were very helpful in clarifying what currently happens, but there
are still some complex issues to discuss.

All QWidgets need to be deleted before QApplication on X11 because
~QApplication closes the X11 display, and QWidget::destroy does not expect
that. This might actually be a bug in Qt/X11, since the behavior is not
documented, does not happen on Windows and there are checks to avoid similar
crashes in ~QWidget; perhaps the call to XFreeGC is simply missing a null
check.

On the other hand, deleting other kinds of objects should not depend on the
existence of a QApplication. If your Qt application has a backend library that
uses QFile instead of Python files, then when you use that library in a non-GUI
context you still expect the QFiles to be flushed, even if you don't create a
QCoreApplication. Note that Python itself does not force the destruction of
file objects, but it relies on the C library to flush its FILEs on exit. So
this means the Python destructor never gets called: perhaps we could do the
same with PySide, simply deleting all remaining QObjects when the Python
interpreter dies. That way Python never has to care about whether the internal
C++ object has been deleted...

In order to solve the first issue, would it make sense to delay the destruction
of QApplication instead of advancing the destruction of QWidget? One way to do
it would be to add a reference from all Shiboken-managed QWidgets to qApp,
since the fact that ~QWidget depends on qApp is a hidden form of dependency
that Python must be made aware of.


Then, the most important question that needs to be answered is: do we want
destructors to only release low-level system resources (which can adequately be
handled by C++ destructors, and looks very much like what plain Python does),
or do we want to allow them to do some application-specific stuff like
requesting some QThreads to stop?

In the latter case, we can't rely on Python's garbage-collection if we NEED all
QWidgets to be deleted even if they are in a cycle, and since as you point out
we can't request Python to forcefully delete an object, we should call both
Python and C++ destructors and replace the object by a dummy object (which is
exactly like what PySide does right now, except that it would include both the
Python and C++ layer instead of just the C++ one).

I don't think there's any reason to avoid calling the __del__ method:
QObject.destroyed can get called too late to be of any use (for example it gets
called after the QThread has been destroyed).

As soon as Python destructors come into play, it is also vital to consider the
order of execution: you wouldn't want to call the destructor of the QThread
before the destructor of the QWidget, for example.
At this point, there are two solutions:
- consider only the QObject children tree
- consider the graph formed by both QObject children and Python references

The issue with the first solution is that it introduces a lot of
non-determinism if the program forgets to make instance variables it uses in
the destructor children of self, potentially causing program crashes
consequently to PySide updates or platform changes.

The issue with the second solution is that it forces us to walk the whole graph
of Python objects, storing a boolean flag for each Python object to remember
whether it was visited. If a number of Python objects were swapped to disk,
this might make program shutdown very slow (but perhaps Python shutdown itself
would be slow in this case?)

(In reply to comment #20)
> Sorry, didn't have time to respond any sooner.
> 
> Your answers were very helpful in clarifying what currently happens, but there
> are still some complex issues to discuss.
> 
> All QWidgets need to be deleted before QApplication on X11 because
> ~QApplication closes the X11 display, and QWidget::destroy does not expect
> that. This might actually be a bug in Qt/X11, since the behavior is not
> documented, does not happen on Windows and there are checks to avoid similar
> crashes in ~QWidget; perhaps the call to XFreeGC is simply missing a null
> check.

I filed a bug on Qt about this issue:

http://bugreports.qt.nokia.com/browse/QTBUG-16974

Anyway Linux is our major target platform and PySide should work well on it, so
if it's a Qt bug consider our current behaviour as a workaround.

> On the other hand, deleting other kinds of objects should not depend on the
> existence of a QApplication. If your Qt application has a backend library that
> uses QFile instead of Python files, then when you use that library in a non-GUI
> context you still expect the QFiles to be flushed, even if you don't create a
> QCoreApplication. Note that Python itself does not force the destruction of
> file objects, but it relies on the C library to flush its FILEs on exit. So
> this means the Python destructor never gets called: perhaps we could do the
> same with PySide, simply deleting all remaining QObjects when the Python
> interpreter dies. That way Python never has to care about whether the internal
> C++ object has been deleted...

Your files will be flushed, QFile destructors always are going to be called and
QFile flushes the contents on the destructor as any other file abstraction I
ever saw in C++. The Python __del__ operator also gets called, but depending on
the context it can be called when the QFile was already deleted or not, this is
the problem, because called it will be, for sure.

> In order to solve the first issue, would it make sense to delay the destruction
> of QApplication instead of advancing the destruction of QWidget? One way to do
> it would be to add a reference from all Shiboken-managed QWidgets to qApp,
> since the fact that ~QWidget depends on qApp is a hidden form of dependency
> that Python must be made aware of.

Delay the destruction of the QApplication is another way to solve the problem
but causes other issues as well and I'm not sure if this solution is really
better than the current one.

> Then, the most important question that needs to be answered is: do we want
> destructors to only release low-level system resources (which can adequately be
> handled by C++ destructors, and looks very much like what plain Python does),
> or do we want to allow them to do some application-specific stuff like
> requesting some QThreads to stop?
> 
> In the latter case, we can't rely on Python's garbage-collection if we NEED all
> QWidgets to be deleted even if they are in a cycle, and since as you point out
> we can't request Python to forcefully delete an object, we should call both
> Python and C++ destructors and replace the object by a dummy object (which is
> exactly like what PySide does right now, except that it would include both the
> Python and C++ layer instead of just the C++ one).

A prefer an exception being throw than an method call that silently does
nothing, so I think having a dummy object isn't a good idea if I understood
correctly.

> [snip]

Well... if the Qt bug gets solved we can forget all this discussion and all
will be magically solved, otherwise we still in the same situation:

* __del__ *always* get called if the Python interpreter wants to (this is why I
think this bug is solved).
* The underlying C++ object may be destroyed before the __del__ gets called if
the QApplication was destroyed beforehand. the C++ object destruction was made
to avoid the the bug QTBUG-16974.

The last item is only thing that can cause complaints, but it's a well definied
behaviour that exists to avoid a crash in Qt and as we saw, get ride of it
isn't easy without a fix for QTBUG-16974.

Hugo, what do you think, should we reopen the bug and mark it WAITING for
QTBUG-16974?

Could be, I'm doing it.

The Qt bug was marked as invalid, the best thing that could happen is it to be
interpreted as a documentation issue, so I'm re-re-closing this bug.

(In reply to comment #21)
> A prefer an exception being throw than an method call that silently does
> nothing, so I think having a dummy object isn't a good idea if I understood
> correctly.

Of course when I said dummy object I meant something that throws an exception
when anything is called, not an object that ignores everything.

I don't think QTBUG-16974 would help solve the problem at all, because we would
still have to delete all QObjects to avoid unflushed QFiles when referenced by
Python cycles, and similar issues with other classes.

My proposal is simple: we should modify Shiboken::Object::destroy to append
code after the GIL locking that:
1. Calls the object's __del__, if any.
2. Clears the Python __dict__ and whatever other variables may need to be
released.
3. Sets a flag in the SbkObject so that it now behaves as an exception-throwing
dummy object.

I can't really see any drawback to this approach, and it would help solve a lot
of problems: destructors are vital in C++ to release various kinds of
resources, and Python still has many resources to worry about (it's critical to
clean up QThreads), so it's logical to give access to this tool to Python
programmers.

Look at your patch in bug 647: you had to do something very dirty to work
around the lack of __del__, namely waiting until the whole app has been closed
to release the resources of a single widget. This doesn't scale well at all.

Of course you can find other workarounds, but you would still end up with
something rather cumbersome or inefficient.

And to clarify: in my proposal, a Shiboken object's __del__ method would never
be called by Python itself, but always by Shiboken::Object::destroy. If Python
decides to garbage-collect the object, it will look up the dummy object's
__del__ method, which should be one of the few methods that does not throw an
exception. The QObject's __del__ method is, of course, never called twice.

Scratch that: __del__ can still get called by Python before destroying the
object, so we should check that it hasn't been called.

Since there still is traffic around this issue, I'm reopening the bug to ensure
it won't get buried away. Re-close if you strongly disagree with this. :-)

I'm also prioritizing this P4: maybe this can wait until after 1.0 to get
fixed?

I'm not sure I agree with this priority: since __del__ gets called with an
invalid object this creates regressions compared to PyQt, as evidenced by the
many examples that had to be manually tweaked. Worse, since this can happen
randomly it can be hard to diagnose, as evidenced by comments #1-#7.

The good news is that I think it's fixable without adding much code.

fixed on commit:

Shiboken:
commit 728fb484a990408394dc56b137e951d11a251d54
Author: Renato Filho <renato.filho@openbossa.org>
Date:   Mon May 23 11:42:14 2011 -0300

PySide:
commit 959aa385f576aca35f86fc04f8336e6a4ce044f1
Author: Renato Filho <renato.filho@openbossa.org>
Date:   Mon May 23 11:39:17 2011 -0300

PySide release 1.0.3