Conversion to wrong SubClass

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Conversion to wrong SubClass

Matthias Kuhn-2
Hi

In QGIS we are facing an issue, where in some cases a wrong subclass
mapping is used in the python bindings.

The code looks like this:

 * There is a parent class QgsAbstractGeometry

 * There are various child classes, among them QgsPoint and QgsMultiPolygon


After heavy operation it can happen, that the python bindings indicate,
that an object is of type QgsPoint, whereas the underlying object is
actually a QgsMultiPolygon (verified that when an overwritten method is
called on the object, the one from QgsMultiPolygon is executed).

It looks like objects which are created subsequently have exactly the
same memory address (see below) and I suspect sip caches type
information with memory addresses somewhere (based on "When SIP needs to
wrap a C++ class instance it first checks to make sure it hasn’t already
done so. " /
<a href="http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode">http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode)

Does sip invalidate this cache information somehow (when the python
wrapper is garbage collected or something else)? Is it possible to
proactively trigger cache invalidation from a destructor? Are there
other ideas what could go wrong and how to deal with that?


** Insights from debugging:

Unconditional debug print statements have been added to

 * QgsAbstractGeometry::QgsAbstractGeometry() (Constructor)
 * QgsAbstractGeometry::~QgsAbstractGeometry() (Destructor)
 * %SipConvertToSubClassCode

grepping the log for the memory address of an affected geometry reveals,
that the constructor is called twice with the object on the same
address, but the SipConvertToSubClassCode is only called once

----------

Constructor: 0x8b1cbd0
SipConvertToSubClassCode - Type QgsPoint: 0x8b1cbd0
Destructor: 0x8b1cbd0
Constructor: 0x8b1cbd0
Destructor: 0x8b1cbd0

---------

Any hints would be much appreciated

Best regards

Matthias

_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Phil Thompson-5
On 18 Jan 2019, at 11:22 am, Matthias Kuhn <[hidden email]> wrote:

>
> Hi
>
> In QGIS we are facing an issue, where in some cases a wrong subclass
> mapping is used in the python bindings.
>
> The code looks like this:
>
>  * There is a parent class QgsAbstractGeometry
>
>  * There are various child classes, among them QgsPoint and QgsMultiPolygon
>
>
> After heavy operation it can happen, that the python bindings indicate,
> that an object is of type QgsPoint, whereas the underlying object is
> actually a QgsMultiPolygon (verified that when an overwritten method is
> called on the object, the one from QgsMultiPolygon is executed).
>
> It looks like objects which are created subsequently have exactly the
> same memory address (see below) and I suspect sip caches type
> information with memory addresses somewhere (based on "When SIP needs to
> wrap a C++ class instance it first checks to make sure it hasn’t already
> done so. " /
> <a href="http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode">http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode)
>
> Does sip invalidate this cache information somehow (when the python
> wrapper is garbage collected or something else)? Is it possible to
> proactively trigger cache invalidation from a destructor? Are there
> other ideas what could go wrong and how to deal with that?
>
>
> ** Insights from debugging:
>
> Unconditional debug print statements have been added to
>
>  * QgsAbstractGeometry::QgsAbstractGeometry() (Constructor)
>  * QgsAbstractGeometry::~QgsAbstractGeometry() (Destructor)
>  * %SipConvertToSubClassCode
>
> grepping the log for the memory address of an affected geometry reveals,
> that the constructor is called twice with the object on the same
> address, but the SipConvertToSubClassCode is only called once
>
> ----------
>
> Constructor: 0x8b1cbd0
> SipConvertToSubClassCode - Type QgsPoint: 0x8b1cbd0
> Destructor: 0x8b1cbd0
> Constructor: 0x8b1cbd0
> Destructor: 0x8b1cbd0
>
> ---------
>
> Any hints would be much appreciated

The cache entry will be invalidated when sip knows the C++ instance has been destroyed. This is easy enough when the instance has been created from Python and has virtual dtors - but you can have C++ APIs which make this all but impossible.. You could look at sip.setdeleted() and similar.

Phil
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Conversion to wrong SubClass

Matthias Kuhn-2
On 1/18/19 12:38 PM, Phil Thompson wrote:

> On 18 Jan 2019, at 11:22 am, Matthias Kuhn <[hidden email]> wrote:
>> Hi
>>
>> In QGIS we are facing an issue, where in some cases a wrong subclass
>> mapping is used in the python bindings.
>>
>> The code looks like this:
>>
>>  * There is a parent class QgsAbstractGeometry
>>
>>  * There are various child classes, among them QgsPoint and QgsMultiPolygon
>>
>>
>> After heavy operation it can happen, that the python bindings indicate,
>> that an object is of type QgsPoint, whereas the underlying object is
>> actually a QgsMultiPolygon (verified that when an overwritten method is
>> called on the object, the one from QgsMultiPolygon is executed).
>>
>> It looks like objects which are created subsequently have exactly the
>> same memory address (see below) and I suspect sip caches type
>> information with memory addresses somewhere (based on "When SIP needs to
>> wrap a C++ class instance it first checks to make sure it hasn’t already
>> done so. " /
>> <a href="http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode">http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode)
>>
>> Does sip invalidate this cache information somehow (when the python
>> wrapper is garbage collected or something else)? Is it possible to
>> proactively trigger cache invalidation from a destructor? Are there
>> other ideas what could go wrong and how to deal with that?
>>
>>
>> ** Insights from debugging:
>>
>> Unconditional debug print statements have been added to
>>
>>  * QgsAbstractGeometry::QgsAbstractGeometry() (Constructor)
>>  * QgsAbstractGeometry::~QgsAbstractGeometry() (Destructor)
>>  * %SipConvertToSubClassCode
>>
>> grepping the log for the memory address of an affected geometry reveals,
>> that the constructor is called twice with the object on the same
>> address, but the SipConvertToSubClassCode is only called once
>>
>> ----------
>>
>> Constructor: 0x8b1cbd0
>> SipConvertToSubClassCode - Type QgsPoint: 0x8b1cbd0
>> Destructor: 0x8b1cbd0
>> Constructor: 0x8b1cbd0
>> Destructor: 0x8b1cbd0
>>
>> ---------
>>
>> Any hints would be much appreciated
> The cache entry will be invalidated when sip knows the C++ instance has been destroyed. This is easy enough when the instance has been created from Python and has virtual dtors - but you can have C++ APIs which make this all but impossible.. You could look at sip.setdeleted() and similar.
>
Yes, in this case it is impossible to automatically detect deletion by
sip because the object is created in C++ code.

From what I can see, sep.setdeleted() works on a Python object. The only
thing we have available in the destructor is a pointer to the C++
object. Is there a way to get the matching python object like done in
the code that calls SipConvertToSubClassCode?

Or does this mean that if the refcount for this Python object dropped to
0 the cache will be invalidated anyway (and the real issue here is, that
the python object is still alive somewhere)?

Matthias

_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Phil Thompson-5
On 18 Jan 2019, at 12:48 pm, Matthias Kuhn <[hidden email]> wrote:

>
> On 1/18/19 12:38 PM, Phil Thompson wrote:
>> On 18 Jan 2019, at 11:22 am, Matthias Kuhn <[hidden email]> wrote:
>>> Hi
>>>
>>> In QGIS we are facing an issue, where in some cases a wrong subclass
>>> mapping is used in the python bindings.
>>>
>>> The code looks like this:
>>>
>>> * There is a parent class QgsAbstractGeometry
>>>
>>> * There are various child classes, among them QgsPoint and QgsMultiPolygon
>>>
>>>
>>> After heavy operation it can happen, that the python bindings indicate,
>>> that an object is of type QgsPoint, whereas the underlying object is
>>> actually a QgsMultiPolygon (verified that when an overwritten method is
>>> called on the object, the one from QgsMultiPolygon is executed).
>>>
>>> It looks like objects which are created subsequently have exactly the
>>> same memory address (see below) and I suspect sip caches type
>>> information with memory addresses somewhere (based on "When SIP needs to
>>> wrap a C++ class instance it first checks to make sure it hasn’t already
>>> done so. " /
>>> <a href="http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode">http://pyqt.sourceforge.net/Docs/sip4/directives.html?highlight=subclass#directive-%ConvertToSubClassCode)
>>>
>>> Does sip invalidate this cache information somehow (when the python
>>> wrapper is garbage collected or something else)? Is it possible to
>>> proactively trigger cache invalidation from a destructor? Are there
>>> other ideas what could go wrong and how to deal with that?
>>>
>>>
>>> ** Insights from debugging:
>>>
>>> Unconditional debug print statements have been added to
>>>
>>> * QgsAbstractGeometry::QgsAbstractGeometry() (Constructor)
>>> * QgsAbstractGeometry::~QgsAbstractGeometry() (Destructor)
>>> * %SipConvertToSubClassCode
>>>
>>> grepping the log for the memory address of an affected geometry reveals,
>>> that the constructor is called twice with the object on the same
>>> address, but the SipConvertToSubClassCode is only called once
>>>
>>> ----------
>>>
>>> Constructor: 0x8b1cbd0
>>> SipConvertToSubClassCode - Type QgsPoint: 0x8b1cbd0
>>> Destructor: 0x8b1cbd0
>>> Constructor: 0x8b1cbd0
>>> Destructor: 0x8b1cbd0
>>>
>>> ---------
>>>
>>> Any hints would be much appreciated
>> The cache entry will be invalidated when sip knows the C++ instance has been destroyed. This is easy enough when the instance has been created from Python and has virtual dtors - but you can have C++ APIs which make this all but impossible.. You could look at sip.setdeleted() and similar.
>>
> Yes, in this case it is impossible to automatically detect deletion by
> sip because the object is created in C++ code.
>
> From what I can see, sep.setdeleted() works on a Python object. The only
> thing we have available in the destructor is a pointer to the C++
> object. Is there a way to get the matching python object like done in
> the code that calls SipConvertToSubClassCode?

At the moment you would have to call sipConvertFromType() and check the reference count of the retruned object.

> Or does this mean that if the refcount for this Python object dropped to
> 0 the cache will be invalidated anyway (and the real issue here is, that
> the python object is still alive somewhere)?

It should be invalidated when the ref count reaches 0.

Phil
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Matthias Kuhn-2
Hi Phil

On 1/18/19 2:13 PM, Phil Thompson wrote:
> On 18 Jan 2019, at 12:48 pm, Matthias Kuhn <[hidden email]> wrote:
>> Or does this mean that if the refcount for this Python object dropped to
>> 0 the cache will be invalidated anyway (and the real issue here is, that
>> the python object is still alive somewhere)?
> It should be invalidated when the ref count reaches 0.

It seems to be doing that. But when an additional /Transfer/ annotation
comes into play things start to go worse.

The following code snippet reproduces the issue:

https://issues.qgis.org/issues/20661#note-2


The interesting bits are the (almost) last two lines of the loop

* pt_g.clone() is a /Factory/ method

* The QgsGeometry constructor has a /Transfer/ annotation on the
parameter, if this one is not called, everything goes well.


Here are the first couple of lines of the output:

------------------------------------------

new pt  0x5d98240
new pt  0x5f257c0
new pt  0x4283da0
new pt  0x5d98240
WHAT??????!!!!!  0x5f257c0
new pt  0x629fa60
new pt  0x5d3c470
WHAT??????!!!!!  0x5d98240
new pt  0x2e2ac90
new pt  0x4283da0
new pt  0x5dcef50
new pt  0x5f770d0
new pt  0x629fa60
WHAT??????!!!!!  0x5d98240

------------------------------------------


Any idea what the additional Transfer of the object triggers?

Best regards

Matthias

_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Phil Thompson-5
On 21 Jan 2019, at 11:13 pm, Matthias Kuhn <[hidden email]> wrote:

>
> Hi Phil
>
> On 1/18/19 2:13 PM, Phil Thompson wrote:
>> On 18 Jan 2019, at 12:48 pm, Matthias Kuhn <[hidden email]> wrote:
>>> Or does this mean that if the refcount for this Python object dropped to
>>> 0 the cache will be invalidated anyway (and the real issue here is, that
>>> the python object is still alive somewhere)?
>> It should be invalidated when the ref count reaches 0.
>
> It seems to be doing that. But when an additional /Transfer/ annotation
> comes into play things start to go worse.
>
> The following code snippet reproduces the issue:
>
> https://issues.qgis.org/issues/20661#note-2
>
>
> The interesting bits are the (almost) last two lines of the loop
>
> * pt_g.clone() is a /Factory/ method
>
> * The QgsGeometry constructor has a /Transfer/ annotation on the
> parameter, if this one is not called, everything goes well.
>
>
> Here are the first couple of lines of the output:
>
> ------------------------------------------
>
> new pt  0x5d98240
> new pt  0x5f257c0
> new pt  0x4283da0
> new pt  0x5d98240
> WHAT??????!!!!!  0x5f257c0
> new pt  0x629fa60
> new pt  0x5d3c470
> WHAT??????!!!!!  0x5d98240
> new pt  0x2e2ac90
> new pt  0x4283da0
> new pt  0x5dcef50
> new pt  0x5f770d0
> new pt  0x629fa60
> WHAT??????!!!!!  0x5d98240
>
> ------------------------------------------
>
>
> Any idea what the additional Transfer of the object triggers?

See the docs.

I can't really comment as you haven't explained the class hierachy or the use of annotations in the ctors.

Does the cloned object have a parent (in which case it's not a factory)?

Phil
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Matthias Kuhn-2
On 1/22/19 10:04 AM, Phil Thompson wrote:

> On 21 Jan 2019, at 11:13 pm, Matthias Kuhn <[hidden email]> wrote:
>> Hi Phil
>>
>> On 1/18/19 2:13 PM, Phil Thompson wrote:
>>> On 18 Jan 2019, at 12:48 pm, Matthias Kuhn <[hidden email]> wrote:
>>>> Or does this mean that if the refcount for this Python object dropped to
>>>> 0 the cache will be invalidated anyway (and the real issue here is, that
>>>> the python object is still alive somewhere)?
>>> It should be invalidated when the ref count reaches 0.
>> It seems to be doing that. But when an additional /Transfer/ annotation
>> comes into play things start to go worse.
>>
>> The following code snippet reproduces the issue:
>>
>> https://issues.qgis.org/issues/20661#note-2
>>
>>
>> The interesting bits are the (almost) last two lines of the loop
>>
>> * pt_g.clone() is a /Factory/ method
>>
>> * The QgsGeometry constructor has a /Transfer/ annotation on the
>> parameter, if this one is not called, everything goes well.
>>
>>
>> Here are the first couple of lines of the output:
>>
>> ------------------------------------------
>>
>> new pt  0x5d98240
>> new pt  0x5f257c0
>> new pt  0x4283da0
>> new pt  0x5d98240
>> WHAT??????!!!!!  0x5f257c0
>> new pt  0x629fa60
>> new pt  0x5d3c470
>> WHAT??????!!!!!  0x5d98240
>> new pt  0x2e2ac90
>> new pt  0x4283da0
>> new pt  0x5dcef50
>> new pt  0x5f770d0
>> new pt  0x629fa60
>> WHAT??????!!!!!  0x5d98240
>>
>> ------------------------------------------
>>
>>
>> Any idea what the additional Transfer of the object triggers?
> See the docs.
>
> I can't really comment as you haven't explained the class hierachy or the use of annotations in the ctors.

To make it easier, I have extracted all the relevant information about
class hierarchy etc. here:

https://gist.github.com/m-kuhn/368c017f05183df707ce054c2f431830


The complete and complex version can be found here:

https://github.com/qgis/QGIS/tree/master/python/core/auto_generated/geometry

>
> Does the cloned object have a parent (in which case it's not a factory)?

There are no QObjects involved here.


My interpretation of what's going on:

* There's a `QgsGeometry` which owns a point `QgsPoint(QgsAbstractGeometry)`

* this point is cloned (ownership of the clone goes to Python at this
point) (/Factory/)

* the clone is then transferred to C++ by calling the `QgsGeomtery(
QgsAbstractGeometry* /Transfer/ )` constructor

* the python wrapper of the clone is garbage collected (that's where I
think cached type information should be cleared)

* then the QgsGeometry is garbage collected and the clone deleted along
(in C++, that's what Python can know nothing about, but by that time any
caching of information regarding the clone should have been invalidated
before if I interpret your previous statements correctly)


Is there something I am missing here?

Matthias


_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: Conversion to wrong SubClass

Elvis Stansvik
Den tis 22 jan. 2019 kl 18:35 skrev Matthias Kuhn <[hidden email]>:

>
> On 1/22/19 10:04 AM, Phil Thompson wrote:
> > On 21 Jan 2019, at 11:13 pm, Matthias Kuhn <[hidden email]> wrote:
> >> Hi Phil
> >>
> >> On 1/18/19 2:13 PM, Phil Thompson wrote:
> >>> On 18 Jan 2019, at 12:48 pm, Matthias Kuhn <[hidden email]> wrote:
> >>>> Or does this mean that if the refcount for this Python object dropped to
> >>>> 0 the cache will be invalidated anyway (and the real issue here is, that
> >>>> the python object is still alive somewhere)?
> >>> It should be invalidated when the ref count reaches 0.
> >> It seems to be doing that. But when an additional /Transfer/ annotation
> >> comes into play things start to go worse.
> >>
> >> The following code snippet reproduces the issue:
> >>
> >> https://issues.qgis.org/issues/20661#note-2
> >>
> >>
> >> The interesting bits are the (almost) last two lines of the loop
> >>
> >> * pt_g.clone() is a /Factory/ method
> >>
> >> * The QgsGeometry constructor has a /Transfer/ annotation on the
> >> parameter, if this one is not called, everything goes well.
> >>
> >>
> >> Here are the first couple of lines of the output:
> >>
> >> ------------------------------------------
> >>
> >> new pt  0x5d98240
> >> new pt  0x5f257c0
> >> new pt  0x4283da0
> >> new pt  0x5d98240
> >> WHAT??????!!!!!  0x5f257c0
> >> new pt  0x629fa60
> >> new pt  0x5d3c470
> >> WHAT??????!!!!!  0x5d98240
> >> new pt  0x2e2ac90
> >> new pt  0x4283da0
> >> new pt  0x5dcef50
> >> new pt  0x5f770d0
> >> new pt  0x629fa60
> >> WHAT??????!!!!!  0x5d98240
> >>
> >> ------------------------------------------
> >>
> >>
> >> Any idea what the additional Transfer of the object triggers?
> > See the docs.
> >
> > I can't really comment as you haven't explained the class hierachy or the use of annotations in the ctors.
>
> To make it easier, I have extracted all the relevant information about
> class hierarchy etc. here:
>
> https://gist.github.com/m-kuhn/368c017f05183df707ce054c2f431830
>
>
> The complete and complex version can be found here:
>
> https://github.com/qgis/QGIS/tree/master/python/core/auto_generated/geometry
>
> >
> > Does the cloned object have a parent (in which case it's not a factory)?
>
> There are no QObjects involved here.
>
>
> My interpretation of what's going on:
>
> * There's a `QgsGeometry` which owns a point `QgsPoint(QgsAbstractGeometry)`
>
> * this point is cloned (ownership of the clone goes to Python at this
> point) (/Factory/)
>
> * the clone is then transferred to C++ by calling the `QgsGeomtery(
> QgsAbstractGeometry* /Transfer/ )` constructor
>
> * the python wrapper of the clone is garbage collected (that's where I
> think cached type information should be cleared)
>
> * then the QgsGeometry is garbage collected and the clone deleted along
> (in C++, that's what Python can know nothing about, but by that time any
> caching of information regarding the clone should have been invalidated
> before if I interpret your previous statements correctly)

"There are two hard things in computer science: cache invalidation,
naming things, and off-by-one errors".

Sorry, could not resist :) Following this mystery with interest, even
if I don't have the knowledge to contribute :/

Elvis

>
>
> Is there something I am missing here?
>
> Matthias
>
>
> _______________________________________________
> PyQt mailing list    [hidden email]
> https://www.riverbankcomputing.com/mailman/listinfo/pyqt
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt