stack_context in iostream.py

[Steve Huffman]

Hey Ben,

I think iostream.py needs to use stack_context. Right now there is a general try/except in _run_callback() that closes the socket if there is any exception raised in the callback, even if that exception would have been handled gracefully in someone else's context.

Here is a changeset that wraps the user callback:

http://github.com/spez/tornado/commit/361362aaba5bb8ceead250743ba36e56f73e21c6

It seems goofy to create the context for just one call, but it does work.

Steve

[Steve Huffman]

After looking into the issue some more, I have another suggestion. What was causing issues is that when multiple requests are being handled at the same time the stack_contexts start to conflict.

The patch I referenced below fixes the issue for me. Also, more simply, wrapping the callback in _run_callback in iostream.py in a NullContext() also fixes the issue (at least it appears to).

My overall concern, however, is that StackContexts aren't the right solution. While wrapping callbacks in AsyncCallback was tedious previously, it was less tedious than wrapping callbacks with stack_context.wrap and then creating and maintaining all the contexts. It was certainly less error-prone.

[Ben Darnell]

On Fri, Oct 1, 2010 at 11:47 PM, Steve Huffman <st...@hipmunk.com> wrote:
> After looking into the issue some more, I have another suggestion. What was
> causing issues is that when multiple requests are being handled at the same
> time the stack_contexts start to conflict.

How so? I understand the problem from your first mail, but don't see
how it's exacerbated by multiple requests.

> The patch I referenced below fixes the issue for me. Also, more simply,
> wrapping the callback in _run_callback in iostream.py in a NullContext()
> also fixes the issue (at least it appears to).
> My overall concern, however, is that StackContexts aren't the right
> solution. While wrapping callbacks in AsyncCallback was tedious previously,
> it was less tedious than wrapping callbacks with stack_context.wrap and then
> creating and maintaining all the contexts. It was certainly less
> error-prone.

StackContext is a deliberate tradeoff to make application code less
error prone at the expense of more complexity in the core libraries.
It's rare for an application to interact with StackContext directly (I
only use it once in Brizzly/Picnics, and I've never had to call
stack_context.wrap outside of Tornado itself).

However, you have highlighted an issue I hadn't thought of, which is
that if any exception handlers are not StackContexts, they not only
may be skipped on subsequent callbacks, but they may be applied out of
order and break things. I don't like the fact that this makes
StackContext effectively mandatory in some cases.

I'm not sure what the right error-handling semantics are for IOStream.
Since HTTPClient is not IOStream-based, most apps only interact with
a single IOStream per request, and that in a relatively limited way.
If HTTPClient used IOStream, would it be right for an uncaught
exception to close both the client and server sockets? Maybe, if the
connection could not be verified to be in a sane state and returned to
the pool, but this suggests that error handling at a deeper level than
IOStream will always be required and IOStream's error handler is just
a last resort.

Error handling is hard. Life would be so much easier if nothing ever failed. :)

-Ben

[Steve Huffman]

Thanks for getting back to me on this.

There are a couple of things that we've been we do on Hipmunk that fall outside normal usage:

- We have lots of IOStreams. We have a memcached library built on IOStream, and we scrape flight results and location info with IOStreams. Each request uses a bunch of streams.

- There are a couple exception-laden portions of our code. There's one spot in particular that uses Exceptions / StackContext to short-circuit out of a lengthy string of callbacks. These exceptions are often being handled in the wrong context.

- I'm trying out using an external ioloop.py. The ZeroMQ python library has a forked version of your ioloop.py that uses ZeroMQ's polling (which uses epoll) to catch ZeroMQ events in addition to regular file descriptor events. This is a separate can of worms, but it's worth noting in case anyone else is attempting to use an outside ioloop.

> After looking into the issue some more, I have another suggestion. What was
> causing issues is that when multiple requests are being handled at the same
> time the stack_contexts start to conflict.

How so?  I understand the problem from your first mail, but don't see
how it's exacerbated by multiple requests.

I'm actually not 100% certain. I can see empirically that when I run simultaneous requests that each create IOStreams to memcached, that one's exception runs in another's context. The context could have been created during a previous request. As I type this, I'm thinking I need to place a NullContext somewhere.

I think what I'm going to do is use my own variation of async_callback in the hairier portions of our code. If I can make things work properly, I'll work backwards into StackContext and hopefully give you better feedback.

Thanks,

Steve

[Steve Huffman]

Ok, I think I can explain things better now.

The source of my troubles is that I have a library built on top of iostream that both takes callbacks and creates its own. The callbacks it is taking (I'll call them incoming-callbacks) as parameters are expected to run in a StackContext, and stack_context.wrapped on the way in, similar to how things work in HttpClient. However, the callbacks it creates (I'll call them local-callbacks) are not run in any context, so when the local-callbacks called the incoming-callbacks and the incoming-callbacks raised an exception, the exception wasn't being handled in the original caller's StackContext.

Phew, I hope this makes sense...

The fix was simple, I just wrapped the code in my library that launches these things in a NullContext, much like HttpClient does. I think the biggest problem here (other than my own mis-understanding of when NullContext should be used) is that iostream closes the socket on an unexpected error, and the re-raising of the exception isn't visible because it gets handled by the original StackContext. Effectively, any exceptions are handled in a backwards order.

I suggest that iostream either at least log the error so the developer can see something is amiss and/or stack_context.wrap incoming callbacks and call them within a NullContext inside _run_callback or handle_events, whichever makes more sense. Is this reasonable?

Steve

[Ben Darnell]

On Wed, Oct 6, 2010 at 8:47 AM, Steve Huffman <st...@hipmunk.com> wrote:
> Ok, I think I can explain things better now.
> The source of my troubles is that I have a library built on top of iostream
> that both takes callbacks and creates its own. The callbacks it is taking
> (I'll call them incoming-callbacks) as parameters are expected to run in a
> StackContext, and stack_context.wrapped on the way in, similar to how things
> work in HttpClient. However, the callbacks it creates (I'll call them
> local-callbacks) are not run in any context, so when the local-callbacks
> called the incoming-callbacks and the incoming-callbacks raised an
> exception, the exception wasn't being handled in the original caller's
> StackContext.
> Phew, I hope this makes sense...
> The fix was simple, I just wrapped the code in my library that launches
> these things in a NullContext, much like HttpClient does. I think the
> biggest problem here (other than my own mis-understanding of when
> NullContext should be used) is that iostream closes the socket on an
> unexpected error, and the re-raising of the exception isn't visible because
> it gets handled by the original StackContext. Effectively, any exceptions
> are handled in a backwards order.
> I suggest that iostream either at least log the error so the developer can
> see something is amiss and/or stack_context.wrap incoming callbacks and call
> them within a NullContext inside _run_callback or handle_events, whichever
> makes more sense. Is this reasonable?

Yes, IOStream._run_callback should definitely log the exception.
Using NullContext doesn't quite feel right here (I had envisioned it
as just being for truly request-independent code like
AsyncHTTPClient._process_queue), but it does kind of seem like the
StackContext magic should happen at the application/tornado boundary.
A NullContext in _add_io_state is probably the right solution.

-Ben

[Ben Darnell]

Try it now with
http://github.com/facebook/tornado/commit/b19953441d88c43b8ba6efd3ed40526c5049d2e3

-Ben

[Steve Huffman]

Aside from the logging, this doesn't change the behavior for me. Granted, things are working nevertheless because I put a NullContext in my own code.

Does wrapping add_handler with NullContext cause handle_events to run in that NullContext? 

[Ben Darnell]

On Sat, Oct 9, 2010 at 11:08 AM, Steve Huffman <st...@hipmunk.com> wrote:
> Aside from the logging, this doesn't change the behavior for me. Granted,
> things are working nevertheless because I put a NullContext in my own code.

Hmm.. Do you think that that NullContext is the right thing to do, or
is it just something that empirically works but you're not sure why?

> Does wrapping add_handler with NullContext cause handle_events to run in
> that NullContext?

Yes, that's the idea. IOLoop and IOStream (and AsyncHTTPClient) exist
outside of any StackContexts and then recreate the contexts when they
return control to the application.

-Ben