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

More error handling #358

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@meejah
Member

meejah commented Mar 12, 2015

This includes unit-tests and fixes for:

  • consistently handle async returns from onLeave etc methods
  • usable, consistent errback API between asyncio/Twisted
  • test-coverage for user-code raising errors
  • test-coverage for exceptions from invoked methods
  • on_progress used consistently everywhere
  • test-coverage for happy-path + exceptions in on_progress handlers

meejah added some commits Mar 11, 2015

Add coverage commands to Makefile + tox.ini
(At least for pypy2 + twisted)
Fixes and unit-tests for errors from ISession user-code
This includes unit-tests and fixes for:

 - handle async returns from onLeave etc methods
 - usable, consistent errback API between asyncio/Twisted
 - test-coverage for user-code raising errors
 - test-coverage for exceptions from invoked methods
 - on_progress used consistently everywhere
except:
typ, exc, tb = sys.exc_info()
if errback:
errback(typ, exc, tb)

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

as said, I won't merge this

# callback and/or errback may be None
# Note that errback takes 3 args: type, exception, tb our
# errbacks can be consistent between Twisted and asyncio.

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

no, both should use 1 argument: the error that happened. is this not the case currently? I don't get it.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

No, currently it's inconsistent: if you're using Twisted, you get a Failure and if you're using asyncio you get just the Exception instance. In this case, you can't print out a traceback (and your error-handler has to do isinstance() or some tricks to figure out if you're in asyncio or Twisted). My previous attempt would work in some cases (e.g. you can call sys.exc_info() but only if onUserError was getting called from inside an except clause, not if it was in an errback)

So, I switched them to the same args that sys.exc_info() returns so that they're a) consistent, and b) you can actually print out a traceback if you need/want to. We could switch to having a single arg (the exception instance, for example) but that's a lot less flexible, IMO (and would still need a wrapper-errback for Twisted to extract the exception from the Failure).

(Also, maybe I'm missing something, but I thought those self._future_*() APIs are intended to be internal-only, right?)

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

Ah, I see. So that's the issue with the current code even: with Twisted, app code gets Failure instead of plain exception. It should be the latter for both. And yes, that requires a Twisted wrapper to unfold the plain exception value. Regardng naming: sorry, yes, the naming suggest private, but it's actually intended as incubation to be made public. Because if apps want to write source compatible code (between Twisted and asyncio), they would be pointed to that API - if we release it.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

Looking at the code in Twisted's Failure, for example, my opinion would be that we should provide at least the 3 args from sys.exc_info() -- otherwise, I don't know of any way that you could print out a stack trace in your error handler.

All you'd be able to do is print out the Exception instance.

As I noted, another alternative would be to provide our own Failure-type object for asyncio (and then in Twisted you could get your Failure and in asyncio you'd get "some object that has many of the same methods"). Not sure if this is a "win" or not though ;)

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

Providing 3 args: doesn't that mean us inventing even a 3rd way of "wrapping" the same information? The other being Twisted failure, and the missing asyncio way.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

All 3 of those bits of info are in the Failure, and we have to provde a Twisted errback-wrapper for either the 3-arg case or the "just the Exception" case anyway.

For asyncio, we immediately have access to those args (we're in "except") and are directly calling the errback() anyway. So, it maps fairly well to how it was already working -- except that you'd get the same thing in the errback no matter if you're in Twisted or asyncio.

The "other" option (our own Failure-type thing for asyncio) means no wrapper-errback for Twisted, but probably more work (although, it could just be a simple data-container for now that just has self.type, self.value and self.tb I guess). It would have the advange for Twisted users of being a more-"normal" single-arg errback too. What I mean is the code would look more like "normal" Twisted code.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

I'd probably lean towards the "Failure-ish thing for asyncio case", but I'm probably really biased since I have more Twisted than asyncio experience ;)

Callback fired when the transport this session will run over has been established.
Callback fired when the transport this session will run over has
been established.

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

if we decide to do that (which we need to discuss first since it is API changing), then we should do that in a different commit. In any case, looking at the impacts takes some thinking and time.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

You're talking about the ability for those ISession methods to be async?

There are some instances in Crossbar that decorate at least onJoin with inlineCallbacks -- I didn't think I was changing the API, just documenting it.

So for example a simple try...catch around self.onJoin calls would be insufficient to catch exceptions from Deferred-returning methods...

e.g. RouterWorkerSession.onJoin is async, as is NativeProcessSession.onJoin in crossbar.

For each ISession method, I'd like to document explicitly which ones can/cannot be async, and can certainly remove any of the async-handling test-cases for ones which shouldn't be. It should be harmless to leave the handling always-async in protocol.py but that could be changed also.

So:

  • onJoin: async
  • onConnect: ?
  • onChallenge: async
  • onLeave: ?
  • onDisconnect: ?

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

Yes, that ability. The onJoin callback (should be named on_join, but that's a different story), can fire multiple times without the underyling transport to go away. Would that be ok with having all callback we allowed to return a deferred/future?

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

Yes, I believe so.

All I'm really changing in this PR is how the error-cases are handled. Currently, even with the try..catch and (recent) onUserError handler we still would simply never log an error if the handler happened to be async.

Now, we will always attach _swallow_error() which will at least log the error (and then ignore it). Of course, this would be a "last-chance" handler -- if user-code put their own error handling inside the onJoin or whatever, they would get called first.

At least, that's the theory ;)

I did add both "returns Deferred" and "simply throws" unit-tests for all the ISession handlers, and covered all the code-paths in protocol.py that lead to those handlers being called.

So: I don't expect any user-visible changes from this PR, except that errors now get logged.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

Shall I add a unit-test for "multiple onJoin calls" just to be explict that's supported (and works)?

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

Yes, splitting it up would be awesome in this case. The asynch callbacks probably right away - you've convinced me on that one already. Rgd. the error thing: I tend to agree about preferring Twisted style;) Probably we could use a namedtuple of just 3 args for asyncio?

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

Another note: not sure, what do you think? Probably the mixin should be named "loop mixins" and be something to be used in various contexts, not just WAMP transport factories or something.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

Okay, I'll make a new pull-request with just the error-handling things (including async/sync case) but not changing the errback or onUserError API.

Separately, I'll play with the errback API and see if I can make something that follows Twisted style (with the real Failures and no errback wrapper) but that also provides a usable API for asyncio case as well.

This comment has been minimized.

@oberstet

oberstet Mar 13, 2015

Member

awesome! sounds great.

This comment has been minimized.

@meejah

meejah Mar 13, 2015

Member

Oh, while I'm thinking of it: do you prefer allowing None as callback or errback to _add_future_callbacks or should I mirror Twisted more and add _add_future_callback and _add_future_errback to the API as well?

For what it's worth, Twisted's addCallbacks allows a None errback but does not allow a None callback (so there'd be no way to add just an errback with such an API -- which is why I let them be None in the first place).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment