Skip to content
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 from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ test_twisted:
USE_TWISTED=1 trial autobahn
#WAMP_ROUTER_URL="ws://127.0.0.1:8080/ws" USE_TWISTED=1 trial autobahn

test_twisted_coverage:
-rm .coverage
USE_TWISTED=1 coverage run --omit=*/test/* --source=autobahn `which trial` autobahn
# coverage -a -d annotated_coverage
coverage html
coverage report --show-missing

# test under asyncio
test_asyncio:
USE_ASYNCIO=1 python -m pytest -rsx
Expand Down
11 changes: 8 additions & 3 deletions autobahn/asyncio/wamp.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

from __future__ import absolute_import

import sys

from autobahn.wamp import protocol
from autobahn.wamp.types import ComponentConfig
from autobahn.websocket.protocol import parseWsUrl
Expand Down Expand Up @@ -102,9 +104,12 @@ def _add_future_callbacks(future, callback, errback):
def done(f):
try:
res = f.result()
callback(res)
except Exception as e:
errback(e)
if callback:
callback(res)
except:
typ, exc, tb = sys.exc_info()
if errback:
errback(typ, exc, tb)
Copy link
Contributor

Choose a reason for hiding this comment

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

as said, I won't merge this

return future.add_done_callback(done)

@staticmethod
Expand Down
36 changes: 29 additions & 7 deletions autobahn/twisted/wamp.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import inspect

from twisted.python import log
from twisted.python.failure import Failure
from twisted.application import service
from twisted.internet.defer import Deferred, \
maybeDeferred, \
Expand Down Expand Up @@ -84,7 +85,29 @@ def _reject_future(future, error):

@staticmethod
def _add_future_callbacks(future, callback, errback):
return future.addCallbacks(callback, errback)
# 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)


# alternatively: what if we made a dummy Failure-like thing
# for asyncio so our "standard" errback in protocol.py could
# use the Failure API, or at least the bits we need...like
# collecting the traceback frames.

def _errback(fail):
# converting to common API between asyncio/Twisted
return errback(fail.type, fail.value, fail.tb)

if callback is None:
assert errback is not None
future.addErrback(_errback)
elif errback is None:
future.addCallback(callback)
else:
future.addCallbacks(callback, _errback)

return future

@staticmethod
def _gather_futures(futures, consume_exceptions=True):
Expand All @@ -96,15 +119,14 @@ class ApplicationSession(FutureMixin, protocol.ApplicationSession):
WAMP application session for Twisted-based applications.
"""

def onUserError(self, e, msg):
def onUserError(self, typ, exc, tb, msg=None):
"""
Override of wamp.ApplicationSession
"""
# see docs; will print currently-active exception to the logs,
# which is just what we want.
log.err()
# also log the framework-provided error-message
log.err(msg)
if msg:
log.err(Failure(exc, typ, tb), msg)
else:
log.err(Failure(exc, typ, tb))


class ApplicationSessionFactory(FutureMixin, protocol.ApplicationSessionFactory):
Expand Down
14 changes: 13 additions & 1 deletion autobahn/wamp/interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,10 @@ class ISession(object):
@abc.abstractmethod
def onConnect(self):
"""
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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: ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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, 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! sounds great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

May return a Deferred/Future.
"""

@abc.abstractmethod
Expand All @@ -276,6 +279,8 @@ def onChallenge(self, challenge):
"""
Callback fired when the peer demands authentication.

May return a Deferred/Future.

:param challenge: The authentication challenge.
:type challenge: Instance of :class:`autobahn.wamp.types.Challenge`.
"""
Expand All @@ -285,6 +290,8 @@ def onJoin(self, details):
"""
Callback fired when WAMP session has been established.

May return a Deferred/Future.

:param details: Session information.
:type details: Instance of :class:`autobahn.wamp.types.SessionDetails`.
"""
Expand All @@ -306,6 +313,8 @@ def onLeave(self, details):
"""
Callback fired when WAMP session has is closed

May return a Deferred/Future.

:param details: Close information.
:type details: Instance of :class:`autobahn.wamp.types.CloseDetails`.
"""
Expand All @@ -320,6 +329,9 @@ def disconnect(self):
def onDisconnect(self):
"""
Callback fired when underlying transport has been closed.

May return a Deferred/Future (but note that the underlying
transport is already gone).
"""

@abc.abstractmethod
Expand Down
Loading