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

perform masks errors #63

Open
rbtcollins opened this issue Aug 16, 2015 · 9 comments
Open

perform masks errors #63

rbtcollins opened this issue Aug 16, 2015 · 9 comments

Comments

@rbtcollins
Copy link
Contributor

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
Copy link
Contributor Author

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

@radix
Copy link
Contributor

radix commented Aug 16, 2015

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
Copy link
Contributor

radix commented Aug 16, 2015

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

@rbtcollins
Copy link
Contributor Author

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?

@radix
Copy link
Contributor

radix commented Aug 17, 2015

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.

@rbtcollins
Copy link
Contributor Author

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

@radix
Copy link
Contributor

radix commented Aug 17, 2015

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.

@rbtcollins
Copy link
Contributor Author

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?

@radix
Copy link
Contributor

radix commented Aug 17, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants