Skip to content

Api callback exceptions #9063

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

Merged
merged 2 commits into from
Aug 24, 2017
Merged

Conversation

tacaswell
Copy link
Member

alternative to #7197

Still needs tests and doc

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 21, 2017
@tacaswell tacaswell added this to the 2.1 (next point release) milestone Aug 21, 2017
@tacaswell
Copy link
Member Author

I also considered defaulting to using the printer, and explicitly passing None or False to get back the existing behavior, could be convinced that is the better path.

@anntzer
Copy link
Contributor

anntzer commented Aug 21, 2017

I think defaulting to the printer is better (with an explicit None to disable it, or perhaps event just lambda exc: None which avoids the need for special casing).

@tacaswell
Copy link
Member Author

Notes from call:

  • how to change API
    • either keep default callback behavior, but we have more to change
    • change default behavior to print is easier on our side, but
      breaks any third-party users who might be using this.
    • Eric and I are both 50/50, anntzer is in favor of changing the
      defaults so will change defaults

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

Looks good. I'm neutral WRT defaulting; maybe just a little in favor of it, as it simplifies things a bit.

cb = CallbackRegistry(exception_handler=None)


A function which take and ``Exception`` as it's only argument may also be passed ::
Copy link
Contributor

Choose a reason for hiding this comment

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

its

@@ -249,9 +249,12 @@ def __hash__(self):
return self._hash


def _excption_printer(exp):
Copy link
Contributor

Choose a reason for hiding this comment

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

typo? (exception)
also I usually use "exc", not "exp", as variable name for an exception

@@ -365,6 +389,13 @@ def process(self, s, *args, **kwargs):
proxy(*args, **kwargs)
except ReferenceError:
self._remove_proxy(proxy)
# this does not capture KeyboardInterrupt, SystemExit,
# and GeneratorExit
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the technically correct way to do this is to store weakref.refs rather than weakref.proxies, so that we can distinguish between failure in dereferencing the callback weakref.ref and failures raised from within the callback.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is good enough for now, we are not doing the right thing in the case where the function we are proxying does not properly handle it's own weak-refs going away.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok to leave this as is for now (we should just drop Py2 and use WeakMethods anyways :-))


A function which take and ``Exception`` as its only argument may also be passed ::

def maybe_reraise(excp):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this def reraise_runtime_errors(exc): ...

Copy link
Member Author

Choose a reason for hiding this comment

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

but it reraises everything but RuntimeError.

exception_handler : callable, optional
If provided must have signature ::

def handler(exception: Exception) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

def handler(exc: Exception) -> None (for consistency with, e.g. the default handler below)

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Would prefer some variables be renamed but that's really not so important.

@tacaswell
Copy link
Member Author

hit 50% of @anntzer 's asks ;)

@anntzer
Copy link
Contributor

anntzer commented Aug 24, 2017

Thanks!
Merging based on @efiring's earlier approval.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants