I took a look through the code and noticed multiple inheritance with the GObject base (GtkScrolledWindow) as a secondary class in the MRO. I'm not sure if this is the problem but it does stick out to me a bit:

class ViewportPlugin(Plugin, gtk.ScrolledWindow):
  def __init__(...)
    Plugin.__init__(self, node, message_manager)
    gtk.ScrolledWindow.__init__(self)


You might try flipping this around to read:

class ViewportPlugin(gtk.ScrolledWindow, Plugin):
  def __init__(...)
    gtk.ScrolledWindow.__init__(self)
    Plugin.__init__(self, node, message_manager)


It would also be helpful to attempt a simplified reproducer of the bug (preferably a failing unittest). The class hierarchy seems to read as follows:

EventMonitor(ViewportPlugin)
ViewportPlugin(Plugin, gtk.ScrolledWindow)
Plugin(Tools)
Tools(object)

Hi Simon,

After investigating this I've just confirmed that the problem is not related to multiple inheritance, but related to the fact that accerciser's Plugin class wraps all callable objects from its plugins (including the callbacks we want to use as handlers for the signals). This is the code wrapping the methods [1]

I'll try to figure out how to avoid this error from Accerciser's source code. If you have any clue/idea about it, don't hesitate to dump your thoughts here.


Best,
Javi

[1]: https://git.gnome.org/browse/accerciser/tree/src/lib/accerciser/plugin/base_plugin.py#n112

Created attachment 268971 [details]
test case

Hi again.

Attached you'll find a test case that reproduces the bug that I've isolated from accerciser.

Hope this helps to clarify what's going wrong with this.

(In reply to comment #3)
> Hope this helps to clarify what's going wrong with this.

It does thanks. The problem is __getattribute__ is wrapping all callable attributes of a plugin instance. This includes the __class__ attribute which also happens to be callable. So when isinstance(plugin, collections.Mapping) is called, it looks at the __class__ attribute and instead of receiving the instances class object, it gets back an instance of _PluginMethodWrapper which is going to be problematic for the Python internals. Basically it breaks duck typing of how an instance should behave:

>>> o = VeryParentClass()
>>> issubclass(o.__class__, VeryParentClass)
TypeError: issubclass() arg 1 must be a class

You could list specific attrs which should not be wrapped:
   if callable(obj) and name not in {'__class__', '__new__', '__init__', ...}:
      ...

or more generally:
   if callable(obj) and not (name.startswith('__') and name.endswith('__')):
      ...

I'm guessing the architecture here is to make the application more robust in regards to plugins which might raise exceptions? The implementation seems like it will have maintenance cost beyond this particular problem. I tend to prefer composition with a proxy like accessor over inheritance:

####

# Plugin API
import traceback
import functools

def logexception(func):
    @functools.wraps(func)
    def newfunc(*args, **kwargs):
        # use Exception otherwise KeyboardInterrupt won't get through
        try:
            return func(*args, **kwargs)
        except Exception:
            traceback.print_exc()
    return newfunc

class Plugin(object):
    def method(self):
        raise NotImplementedError

class PluginAccessor(Plugin):
    def __init__(self, plugin):
        self.plugin = plugin

    # Explicitly wrap each function of the plugin API (preferred approach)
    @logexception
    def method(self):
        return self.plugin.method()

    # Alternatively use __getattr__ for wrapping all non-builtin attributes
    # which is safer than using __getattribute___
    def __getattr__(self, name):
        func = getattr(self.plugin, name)
        if callable(func):
            return logexception(func)
        return func

# Some external plugin
class MyPlugin(Plugin):
    def method(self):
        raise ValueError('unintended plugin error')

    def other_method(self):
        raise ValueError('another unintended plugin error')


# Always work with a plugin instance through a PluginAccessor
# this should also be usable when passed to Builder.connect_signals
accessor = PluginAccessor(MyPlugin())
accessor.method()
accessor.other_method()
print(issubclass(accessor.__class__, Plugin))  # True
print("exceptions in plugins won't bubble to the main program")

####

Hi again.

(In reply to comment #4)
> (In reply to comment #3)
> > Hope this helps to clarify what's going wrong with this.
> 
> It does thanks. The problem is __getattribute__ is wrapping all callable
> attributes of a plugin instance. This includes the __class__ attribute which
> also happens to be callable. So when isinstance(plugin, collections.Mapping) is
> called, it looks at the __class__ attribute and instead of receiving the
> instances class object, it gets back an instance of _PluginMethodWrapper which
> is going to be problematic for the Python internals. Basically it breaks duck
> typing of how an instance should behave:
> 
> >>> o = VeryParentClass()
> >>> issubclass(o.__class__, VeryParentClass)
> TypeError: issubclass() arg 1 must be a class
> 
> You could list specific attrs which should not be wrapped:
>    if callable(obj) and name not in {'__class__', '__new__', '__init__', ...}:
>       ...
> 
> or more generally:
>    if callable(obj) and not (name.startswith('__') and name.endswith('__')):
>       ...
> 

You're absolutely right, I didn't realized that attributes such as __class__ were being wrapped too (and its consequences) :/

Now I can get the code working just by not wrapping the __class__ attribute.


> I'm guessing the architecture here is to make the application more robust in
> regards to plugins which might raise exceptions? 

Yes, that's it. We wrap all callable objects from plugins in order to raise the exceptions in an homogeneous way.


> The implementation seems like it will have maintenance cost beyond this particular problem. I tend to prefer
> composition with a proxy like accessor over inheritance:
> 
> ####
> 
> # Plugin API
> import traceback
> import functools
> 
> def logexception(func):
>     @functools.wraps(func)
>     def newfunc(*args, **kwargs):
>         # use Exception otherwise KeyboardInterrupt won't get through
>         try:
>             return func(*args, **kwargs)
>         except Exception:
>             traceback.print_exc()
>     return newfunc
> 
> class Plugin(object):
>     def method(self):
>         raise NotImplementedError
> 
> class PluginAccessor(Plugin):
>     def __init__(self, plugin):
>         self.plugin = plugin
> 
>     # Explicitly wrap each function of the plugin API (preferred approach)
>     @logexception
>     def method(self):
>         return self.plugin.method()
> 
>     # Alternatively use __getattr__ for wrapping all non-builtin attributes
>     # which is safer than using __getattribute___
>     def __getattr__(self, name):
>         func = getattr(self.plugin, name)
>         if callable(func):
>             return logexception(func)
>         return func
> 
> # Some external plugin
> class MyPlugin(Plugin):
>     def method(self):
>         raise ValueError('unintended plugin error')
> 
>     def other_method(self):
>         raise ValueError('another unintended plugin error')
> 
> 
> # Always work with a plugin instance through a PluginAccessor
> # this should also be usable when passed to Builder.connect_signals
> accessor = PluginAccessor(MyPlugin())
> accessor.method()
> accessor.other_method()
> print(issubclass(accessor.__class__, Plugin))  # True
> print("exceptions in plugins won't bubble to the main program")
> 
> ####

Totally agree. This looks like a better approach to me so I'll try to move the code ASAP.

Thanks fo helping me on this.
Take care!