Skip to content

Catch exceptions that occur in callbacks. #7197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 29, 2016

Uncaught exceptions are fatal to PyQt5 so we catch anything that occurs
in a callback of the canvas class.

e.g.

from matplotlib import pyplot as plt

plt.gca().figure.canvas.mpl_connect(
    "axes_enter_event", lambda event: 1 / 0)
plt.show()

used to crash matplotlib upon entering the axes.

More general than #7052.

Uncaught exceptions are fatal to PyQt5 so we catch anything that occurs
in a callback of the canvas class.

e.g.
```
from matplotlib import pyplot as plt

plt.gca().figure.canvas.mpl_connect(
    "axes_enter_event", lambda event: 1 / 0)
plt.show()
```
used to crash matplotlib upon entering the axes.
@@ -542,6 +542,21 @@ def process(self, s, *args, **kwargs):
except ReferenceError:
self._remove_proxy(proxy)

def safe_process(self, s, on_error, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any changes you will be calling with an on_error different from traceback.print_exc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, close_event completely silences out specific exception types.

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Sep 29, 2016
@tacaswell
Copy link
Member

We vendored and modified the callback registry at my day-job (https://github.com/NSLS-II/bluesky/blob/master/bluesky/utils.py#L159) to add a stateful flag to the control if callbacks are allowed to raise or not.

I think that is a better option here as it would require less change to the code base, provides a way to go back to the previous behavior, and does not leave an 'orphaned' method on the callback registry.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 29, 2016

The problem of the flag you propose is that exceptions are completely swallowed. An alternative would be to replace the flag by a callback stored as an attribute... say traceback.print_exc for example :-)

Side note for @tacaswell: for your own implementation, given that it is Py3 only you could probably use weakref.WeakMethod instead of having to implement your own _BoundMethodProxy. (And we could do the same once mpl becomes Py3 only...)

@tacaswell
Copy link
Member

@anntzer I agree, that is a better idea than a flag.

@dopplershift
Copy link
Contributor

👍 to the idea of using a callable attribute

@anntzer
Copy link
Contributor Author

anntzer commented Oct 3, 2016

The problem is that we need a half-decent method for locally patching this attribute when needed. However, the only place where this needs to happen is in close_event:

    def close_event(self, guiEvent=None):
        """
        This method will be called by all functions connected to the
        'close_event' with a :class:`CloseEvent`
        """
        s = 'close_event'
        def on_error():
            # Suppress the TypeError when the python session is being killed.
            # It may be that a better solution would be a mechanism to
            # disconnect all callbacks upon shutdown.
            # AttributeError occurs on OSX with qt4agg upon exiting
            # with an open window; 'callbacks' attribute no longer exists.
            tp, value, traceback = sys.exc_info()
            if not isinstance(value, (TypeError, AttributeError)):
                traceback.print_exc()
        event = CloseEvent(s, self, guiEvent=guiEvent)
        self.callbacks.safe_process(s, on_error, event)

(the earlier version wrapped self.callbacks.process to catch these exception specifically). This handling seems to have been introduced by #1098/#1184; I cannot reproduce the issue in #1184, and #1098 is commented as being MacOS only. Can someone with a mac check whether we can get rid of the special silencing there too?

@tacaswell
Copy link
Member

The AttributeError would come out before the process method is called anyway (as it is complaining that callbacks no longer exists). Not sure about the origin of the TypeError though.

@QuLogic QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017
@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) Jul 11, 2017
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 11, 2017
@tacaswell tacaswell self-assigned this Jul 25, 2017
@tacaswell
Copy link
Member

Closing in favor of #9063

@tacaswell tacaswell closed this Aug 21, 2017
@anntzer anntzer deleted the catch-exns-in-callbacks branch August 31, 2017 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: needs revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants