Request for code review: Tornado AsyncResult

[A. Jesse Jiryu Davis]

Hi all, I'm starting a cute side-project called "Toro" where I adapt Python's and Gevent's synchronization primitives to Tornado. The first primitive is Gevent's AsyncResult. My API is like:

class AsyncResult:
    def get(self, callback, timeout):
        """Get the value that's been set.
        If callback provided, execute it with the value after set() is run, or execute it
        with None after timeout."""

    def set(self, value):
        """Set the value, wake all waiters."""

The idea is for an async function to "block" waiting for another function to set a result:

@gen.engine
def get_result(async_result):
    value = yield gen.Task(async_result.get)
    do_something_with(value)

Would some Tornado expert like to code-review my implementation so far, particularly regarding exception-handling? It's just a 60-line file and a 30-line test:

https://gist.github.com/3877210

I'm particularly anxious to know if people think my exception-handling test expects the proper behavior, and if my use of stack_context.wrap() on line 69 is correct:

https://gist.github.com/3877210#L69

[Ben Darnell]

Looks good to me.

[Roey Berman]

There's one issue with your stack_context handling.

It's good that you stack_context.wrap() the callback when setting it in _Waiter.

You'll have to call that callback inside a NullContext() to prevent a stack_context leak in set().

Take this scenario for example.

set()'s ExceptionStackContext will catch the exception in raise_callback().

---

        def set_result():

            with stack_context.ExceptionStackContext(catch_set_result_exception):

                result.set('hello')

        def callback(value):

            outcomes['value'] = value

            loop.add_callback(raise_callback)

        def raise_callback():

            assert False

        def catch_set_result_exception(type, value, traceback):

            outcomes['set_result_exc'] = type

            return True

        result.get(callback)

        loop.add_timeout(time.time() + .01, set_result)

        loop.start()

        self.assertEqual(outcomes['value'], 'hello')

        self.assertNotEqual(outcomes['set_result_exc'], AssertionError)

[Ben Darnell]

Good catch. This is actually changing in Tornado 3.0, so you won't
need the NullContext in the future. This leak happens when a callback
wrapped with no contexts is run under a StackContext; if both the
calls to get and set had separate contexts the leak wouldn't happen.

-Ben

[A. Jesse Jiryu Davis]

Brilliant, thanks Roey. I've updated:

https://gist.github.com/3877210/

I knew I was probably missing something. =)