brainvisa / axon

Brainvisa main GUI
Other
0 stars 1 forks source link

Uncatched exceptions that occurs in BrainVISA lead to a crash of the application #46

Closed nsouedet closed 3 years ago

nsouedet commented 3 years ago

Since 5.0 release, trying to load an old bvproc for which parameter names have changed in the process lead to a crash of the application (parseParameterString raises a KeyError that is never catched). I believe that some exception's global catching have been removed in order to not hide issues and to have a correct return code when the command fails. However, the axon GUI closes violently more often. What could we do to avoid this ?

denisri commented 3 years ago

I don't reproduce this behaviour but maybe my trials did not trigger the same error. Anyway I guess this behaviour change can possibly be explained by the move to PyQt5: in PyQt4 when an exception was raised within a Qt slot callback, the GUi was continuing (the error was just printing a message). In Qt5 an exception in a slot will abort the application, as you describe. But I can't be sure of it until I have a test case, or a traceback... If the cause is PyQt5, we have found no general way to solve this: we have to put try..except blocks in every slot which might raise an exception...

denisri commented 3 years ago

Do you have a test case for this ?

nsouedet commented 3 years ago

Ok, sorry this bug was not correclty qualified. I can reproduce it with an old .bvproc, but it seems to occur not because a parameter was renamed. It seems to occur when a proces does not exists anymore. I am trying to reproduce it with a simple (and clearer) case. The traceback is the following :

Traceback (most recent call last):
  File "/volatile/ns215310/development/svn/brainvisa_bioproj/brainvisa/build/Ubuntu-16.04-x86_64/trunk/release/python/brainvisa/processing/qt4gui/neuroProcessesGUI.py", line 3600, in open
    showProcess(brainvisa.processes.getProcessInstance(minf))
  File "/volatile/ns215310/development/svn/brainvisa_bioproj/brainvisa/build/Ubuntu-16.04-x86_64/trunk/release/python/brainvisa/processes.py", line 4719, in getProcessInstance
    + '": invalid identifier or process file: ' + repr(e))
KeyError: 'Could not get process "u\'/home/Petri/BrainVISA/3D-HAPi/3D-HAPi-test-dataset.bvproc\'": invalid identifier or process file: IOError(2, \'No such file or directory\')'
nsouedet commented 3 years ago

The following bvproc file allows to reproduce the problem : test.zip

nsouedet commented 3 years ago

The segfault is related to Qt5 slots. The simplest is to catch the exception and display it in brainvisa errors. But I guess there are many other Qt5 slots that not always catch exception and that can lead to segmentation faults. A way to globaly address the problem could be to add a decorator (may be named "catchExceptions") to catch exceptions in qt slots? Nevertheless It will be necessary to add this decorator to each slot that can raise exception.

denisri commented 3 years ago

We could try that, yes. I can reproduce the problem now.

denisri commented 3 years ago

The decorator trick works more or less but has a side effect: many signals in Qt have optional arguments, such as QAction::triggered(checked=False). When they are connected to a slot which does not define this argument, then they are called without it, otherwise they are called with it. If the slot is a generic decorated function which takes (*args, **kwargs) as parameters, then the signal cannot know if it should define the optional arguments or not, and here it does. Thus slots are not called the same way as they used to be called. For instance ProcessView.open takes no arguments. With the decorator, it gets called with False as parameter, and it doesn't work any longer. We can perhaps modify the decorator to make it instrospect the function (if it is possible, it is not always possible), but it's not as simple as that.

denisri commented 3 years ago

Using inspect seems to work... I'm doing that.

nsouedet commented 3 years ago

Ok, thanks. For me, the following code allows to decorate functions with optional arguments and I wonder if functools.wraps is not able to fix the problem you mention :

def default_catch_function():
    import sys
    import traceback

    e, v, t = sys.exc_info()
    try:
        name = e.__name__
    except AttributeError:
        name = str(e)
    print(''.join(line for line in traceback.format_exception(e, v, t)))

def catchExceptions(func=None, catch_func=default_catch_function):
    def decorator_catchExceptions(_func):
        import functools
        @functools.wraps(_func)
        def wrapper(*args, **kwargs):
            try:
                return _func(*args, **kwargs)
            except Exception:
                if catch_func is not None:
                    catch_func()
        return wrapper

    if func is None:
        return decorator_catchExceptions
    else:
        return decorator_catchExceptions(func)

@catchExceptions
def qt_slot(arg1, optional="test"):
    print('arg1:', arg1)
    print('optional:', optional)
    raise RuntimeError('This is a runtime error')

try:
    qt_slot('1st value', 'my optional test')
except Exception:
    print('!! Exceptions were not catched properly')

print('!! End')
denisri commented 3 years ago

I didn't know this functools.wraps, that seems exactly what I was missing... I'll modify the code to use it. Thanks...

denisri commented 3 years ago

Well, I don't know what this functools.wraps does actually, but it doesn't seem to work in our case, which is much more like this:

from PyQt5 import Qt

def default_catch_function():
    import sys
    import traceback

    e, v, t = sys.exc_info()
    try:
        name = e.__name__
    except AttributeError:
        name = str(e)
    print(''.join(line for line in traceback.format_exception(e, v, t)))

def catchExceptions(func=None, catch_func=default_catch_function):
    def decorator_catchExceptions(_func):
        import functools
        @functools.wraps(_func)
        def wrapper(*args, **kwargs):
            try:
                return _func(*args, **kwargs)
            except Exception:
                if catch_func is not None:
                    catch_func()
        return wrapper

    if func is None:
        return decorator_catchExceptions
    else:
        return decorator_catchExceptions(func)

@catchExceptions
def qt_slot2():
    print('SLOT 2')

app = Qt.QApplication([])
w = Qt.QToolButton()
ac = Qt.QAction('push')
ac.triggered.connect(qt_slot2)
w.setDefaultAction(ac)
w.show()

app.exec_()

This code produces the following when clicking the button:

Traceback (most recent call last):
  File "test2.py", line 20, in wrapper
    return _func(*args, **kwargs)
TypeError: qt_slot2() takes 0 positional arguments but 1 was given

in other words, the Qt signal (with an optional parameter checked=False) could not determine that the slot has no parameter at all, and should be called without the optional parameter. In the version I pushed (using inspect and several cases) it works. However maybe the functools.wraps could be used additionally to document parameters (rather than *args, **kwargs) (?)

nsouedet commented 3 years ago

Ok, sorry, I hoped it was possible to use it in our case.

denisri commented 3 years ago

Well it seemed to be what we were looking for, but it was not ;)