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

[drdaeman]

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 (57f81034eb4817357117efb2909ea09feb90944c).

[drdaeman]

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

[kmike]

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.

[evilkost]

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

[drdaeman]

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

[evilkost]

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.