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

Already on GitHub? Sign in to your account

When callback throws an exception, callback's (wrongly) called again #7

Closed
drdaeman opened this Issue Apr 14, 2011 · 5 comments

Comments

Projects
None yet
3 participants

When callback throws an exception (like HTTPError) this callback's called again by brükva, with the exception as argument.

A simple example (with a kludgy workaround) which reproduces the problem:

#!/usr/bin/env python

import tornado.ioloop
import tornado.web
import brukva

rc = brukva.Client()
rc.connect()

class MainHandler(tornado.web.RequestHandler):
    @tornado.web.asynchronous
    def get(self):
        rc.get("no-such-key", self._get_1)

    def _get_1(self, data):
        # BUG: The following condition feels wrong
        if isinstance(data, tornado.web.HTTPError):
            print("BUG: We're being called with self-thrown exception")
            raise data
        if data is None:
            raise tornado.web.HTTPError(404)
        self.write(data)
        self.finish()

application = tornado.web.Application([
    (r"/", MainHandler),
])

if __name__ == "__main__":
    application.listen(8888)
    tornado.ioloop.IOLoop.instance().start()

The problem is, brükva catches exception, thinks it happened in its own internals and sends it to callbacks at forward_error:29.

Tested with current HEAD (57f8103).

Possible solution: drdaeman/brukva@de1c52fae9b3045e4b75c465c77a26bddd4bb259 (the code somehow feels dirty, though)

kmike commented Apr 14, 2011

It seems more clear to catch only brukva's exceptions (not all exceptions) at forward_error context manager. Inherit brukva exceptions from 1 base class and wrap other exceptions (IOError?) into brukva's exception if it is necessary.

Though I must admin I don't fully understand how error handling works now: why is exceptions passed back to callbacks and what are these callbacks. Can somebody provide high-level description on what is error handling supposed to be? This description can be put into source code comments then.

Owner

evilkost commented Apr 15, 2011

2drdaeman
Yep, my bad. Callbacks was wrongly called with exception, because in function like Client.execute self.call_callbacks... was placed inside 'with' statement. Simple fix: call_callbacks shift outside from with exception.
One special case is Client.listen, so i added disable&enable method to forward_error context (FYI: evilkost/brukva@3d15814e8 )

2kmike
About error handling. I wanted to eliminate code like this:

...
(error, value) = yield client.async.get('foo')
if error:
    handle_error(error)
else:
    (error, other_value) = yield client.async.get('bar')
    if error:
        handle_error(error)
...

I patched brukva.adisp:110-113 6f5514c1 to throw exception from generator if value is instance of Exception.
So now, it look much better:

...
try:
    value = yield client.async.get('foo')
    other_value = yield client.async.get('bar')
except Exception, e:
    handle_error(error)
...

And with context manager forward_error is just syntax sugar:

...
with forward_error(handle_error):
    value = yield client.async.get('foo')
    other_value = yield client.async.get('bar')   
...

Simple fix: call_callbacks shift outside from with exception.

No, just shifting the indentation won't do it. While this fixes the original bug, it'll introduce another one — a callback will be called twice, first with the exception, then with some "failure" result.

This is debatable, but I believe callback're expected to be called only once per request — unless a command is a special case like listen. Therefore call_callbacks should be normally called only when no exception occured. This is why I used the try...except...else. An example illustrating the problem:

#!/usr/bin/env python

import contextlib

@contextlib.contextmanager
def chew_error(callback):
    try:
        yield callback
    except Exception, e:
        callback(e)

def callback(result):
    print("Got {0!r}".format(result))

def example():
    result = "failure"
    with chew_error(callback):
        raise Exception, "exception"
        result = "ok"
    callback(result)

if __name__ == "__main__":
    example()
Owner

evilkost commented Apr 15, 2011

Oops.
Although, if shift back callback calling inside with and restrict error handling, example will be working.
But i think, it's not clean.

def callback(result):
    print("Got {0!r}".format(result))

def example():
    result = "failure"
    with forward_error(callback) as forward:
        raise Exception, "exception"
        result = "ok"
        forward.disable() # restrict error handling
        callback(result)

Or add special method to forward_context

class ForwardErrorManager(object):
    def ret(self, result):
        self.disable()
        self.callbacks(result)
        self.enable()


def example():
    result = "failure"
    with forward_error(callback) as forward:
        raise Exception, "exception"
        result = "ok"
        forward.ret(result)

I should think for some time.

@evilkost evilkost closed this in d47eff4 Apr 16, 2011

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