effect icon indicating copy to clipboard operation
effect copied to clipboard

perform masks errors

Open rbtcollins opened this issue 10 years ago • 9 comments

This is perhaps a usage bug rather than a bug in effect, but:

class Print:
    def __init__(self, line):
        self.line = line


@sync_performer
def real_print(dispatcher, print_):
    print(print_.line)
    import pdb;pdb.Pdb(stdout=sys.stderr).set_trace()
    sys.stdout.flush()


real_interpreter = ComposedDispatcher([
    TypeDispatcher({Print: real_print}),
    base_dispatcher])


def program():
    return Effect(Print('What... is your quest?'))


if __name__ == '__main__':
    perform(real_interpreter, program())
$ python 05.py > /dev/full
$ echo $?
0

It seems reasonable to me that in the absence of a error catcher anywhere that it should ultimately propogate up to perform and beyond.

rbtcollins avatar Aug 16 '15 08:08 rbtcollins

This is exacerbated by the bare except in sync_wrapper that makes even raising SystemExit be insufficient.

rbtcollins avatar Aug 16 '15 08:08 rbtcollins

You probably want to be using sync_perform, not perform.

Perhaps I should have made perform have the behavior of sync_perform by default, and called the core function async_perform. I can't think of a way off the top of my head to swap those around without breaking compatibility.

radix avatar Aug 16 '15 21:08 radix

(also note the difference between sync_perform and sync_performer. Sorry.)

radix avatar Aug 16 '15 22:08 radix

Aieee.

So, first step would be to make the docs steer me to the things you're recommending :)

Secondly - I know twisted's model is to throw uncaught exceptions on the floor. I think this is batshit.

Lets assume for the moment that it's desirable for perform to propogate unhandled exceptions. Do you think its technically feasible?

rbtcollins avatar Aug 17 '15 00:08 rbtcollins

Yeah, I think at the very least the docs should show usage of sync_perform instead of perform.

As far as having perform raising errors, that's not possible in general. sync_perform is specifically designed to be used in the synchronous case where you do want either the result or an exception synchronously.

Basically since Effect supports both asynchronous or synchronous performers, it's explicitly designed asynchronously since that's the lowest-common-denominator. Then sync_perform and @sync_performer are used to bring it back into the synchronous world.

radix avatar Aug 17 '15 20:08 radix

Can you help me understand why the async case can't raise errors?

rbtcollins avatar Aug 17 '15 20:08 rbtcollins

Yeah, sure. The problem is that most of the time (when things are actually asynchronous) the exceptions happen after perform() has returned.

Now that I think of it it may be feasible for perform() to raise an exception if it knows that the Effect was synchronous and already has a result. Hmm. It could even return the result if it's already available -- in which case it's basically just doing what sync_perform does but without raising an exception if the effect is asynchronous.

radix avatar Aug 17 '15 21:08 radix

So sure, if the error happens after perform returns, clearly thats hard :). But, if perform does achieve a final result - either because the outer most effect was sync, or the result was already available, then it could do it?

That might make the distinction between sync_perform and perform less significant?

rbtcollins avatar Aug 17 '15 21:08 rbtcollins

yep. sounds good. It'll technically backwards incompatible because perform may start raising exceptions when previously it wasn't. My gut feeling is that it should be fine especially because very few people are using Effect so far.

radix avatar Aug 17 '15 21:08 radix