[QGIS-Developer] Is it just me, or is sip totally broken?

[Matthias Kuhn]

Hi all,

I have observed the same in the past.

This explanation sounds very reasonable, Martin.

Are you convinced that the proper solution will be a /ReleaseReference/ 
annotation?

I would assume that the current information is sufficient for sip / 
sipify to generate proper bindings without a new annotation.

 From what I understand, sip creates an internal subclass that forwards 
virtual methods to their python implementations. I would assume that the 
destructor of this subclass can be used to trigger the garbage 
collection / decreasing the ref counter and the reference counter should 
be increased when the object is sent through a /Transfer/ annotated 
function parameter.

If that works, there's still work to be done, but the fix wouldn't 
affect the QGIS source itself.

Matthias

On 07.01.19 12:11, Martin Dobias wrote:
> Hi Nyall
>
> I have seen that too with other QGIS registries, but I am not sure if
> it is actually broken in SIP or if this should be simply handled in a
> different way.
>
> My understanding is that SIP for an object has the "c++ part" and
> "python part". If we do not deal with subclasses, things seem to work
> nicely: if you pass object of type X to function "myfunc(X* x
> /Transfer/)" then the object gets ownership passed to C++, and then
> when in python the object gets out of scope, its "python part" get
> deleted because it is not necessary anymore and only the "c++ part"
> keeps on living (and will be deleted in c++ code. The trouble is when
> passing derived classes with /Transfer/ where the "python part" is
> actually important, but it still gets deleted.
>
> SIP also has /KeepReference/ annotation, but it's probably not that
> useful here, because it is meant to keep reference to one python
> object passed through API (if I had a function addCheck with
> /KeepReference/ arg annotation then the first call to addCheck(a) will
> keep the reference, but the second call addCheck(b) would drop
> reference to "a"). The trouble is that if we want SIP hold reference
> to the python object, we would need to give it extra information about
> the object's life cycle (when is it safe to release the reference). So
> we would probably need a /KeepReference/ variant with a matching
> /ReleaseReference/ annotation for some other function - like this SIP
> could track of multiple objects (which is the case of registries) and
> handle their life cycle correctly. Maybe this registry pattern is not
> used anywhere in Qt so SIP does not include support for it...
>
> A simple "fix" is to just keep the reference to the object in the
> python code. Not the nicest solution I know...
>
> Cheers
> Martin
>
> On Thu, Jan 3, 2019 at 8:19 AM Nyall Dawson <nyall.dawson at gmail.com> wrote:
>> Hi list,
>>
>> I'm at my wits end here, fighting with a sip issue I can't solve. And
>> honestly, I'm starting to think maybe there's something completely
>> broken in sip. (Or at least, in the version 4.19.7 provided by my
>> distro).
>>
>> Here's what I'm currently hitting:
>>
>> The validity check registry added this week has a method "addCheck",
>> which transfers ownership of the check to c++:
>>
>> void addCheck( QgsAbstractValidityCheck *check SIP_TRANSFER );
>> https://github.com/qgis/QGIS/blob/master/src/core/validity/qgsvaliditycheckregistry.h#L65
>>
>> But if I make a Python subclass of QgsAbstractValidityCheck, and add
>> it to the registry, it's immediately garbage collected by Python!
>>
>> My test code looks like this:
>>
>> class LayoutMapCrsCheck(QgsAbstractValidityCheck):
>>
>>      def __del__(self):
>>          print('WTFFFFFFF!!!')
>>
>>      def id(self):
>>          return 'map_crs_check'
>>
>>      def checkType(self):
>>          return QgsAbstractValidityCheck.TypeLayoutCheck
>>
>>      def prepareCheck(self,context, feedback):
>>          return True
>>
>>      def runCheck(self,context, feedback):
>>          return []
>>
>> def add_check():
>>      QgsApplication.validityCheckRegistry().addCheck(LayoutMapCrsCheck())
>>
>> add_check()
>>
>>
>> So I create a Python subclass of the interface, and then add it to the
>> registry. And boom - it's immediately deleted.
>>
>> If I make a global copy of the check, and then add to the registry,
>> everything works -- the check isn't garbage collected.
>>
>> check_instance = LayoutMapCrsCheck()
>> QgsApplication.validityCheckRegistry().addCheck(check_instance)
>>
>> But this leads me to believe that sip is broken somewhere, and is
>> ignoring the /Transfer/ annotation.
>>
>> Indeed - I've also seen this in other classes. Specifically
>> QgsTaskManager.addTask() -- Python tasks must be stored globally in
>> order to avoid them being immediately deleted too.
>>
>> Can anyone else reproduce using my above code? Does anyone have any
>> ideas what's happening here?
>>
>> Nyall
>> _______________________________________________
>> QGIS-Developer mailing list
>> QGIS-Developer at lists.osgeo.org
>> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
> _______________________________________________
> QGIS-Developer mailing list
> QGIS-Developer at lists.osgeo.org
> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>