Hello Guido, et. al., Replies below...
On Sat, Dec 19, 2015 at 11:11 PM Guido van Rossum <[email protected]> wrote: > Hi Justin, > > I have to apologize for dropping the ball here. Unfortunately I'm not > going to be able to pick it up soon (I'm going to be in Texas for family > time for the next two weeks). > No worries, I'm in the same boat; Spokane and Montana though. > > I do have one thought about KeyboardInterrupt. This is a very annoying > exception for a principled app, because it arrives asynchronously, which > means it can interrupt things that should not be interruptable (however, > finally clauses are still executed, unless the signal arrives in the midst > of one -- then things may get worse). > > There is also good news though: you can specify a signal handler (at least > on Unixoid systems) for SIGINT that cooperates with the event loop, using > the loop.add_signal_handler() call. I can't tell whether this would be an > acceptable solution for your library, but if you don't do this, you really > have to accept that sometimes ^C will break stuff in a bad way. A > well-aimed ^C could break out of a finally clause (or a with-statement's > __exit__ call) without performing the intended cleanup. > I've definitely considered this and there is a good chance I'll end up doing this in addition to the work I may need to do for handling GC'd generators (discussed below). > > The other BaseExceptions worth considering are: > > - SystemExit, raised by os.exit(). This is typically self-inflicted damage > -- who in their right mind would call this in the middle of a library > function? > I'm getting a good laugh out of this. I have a library that wraps argparse and I have to catch SystemExit in all sorts of places to avoid the interpreter quitting on me because argparse throws SystemExit when it can't parse an argument string. It's not really germane to this topic but made me laugh nonetheless. > Yet it also probably means that whoever wrote that call really meant for > the process to exit right there (maybe because some unrecoverable breakage > of an invariant occurred). Interestingly, in a non-main thread, it doesn't > exit the process -- it exits the thread only. So libraries that call > os.exit() or raise SystemExit are already broken in multi-threaded > environments. > > - GeneratorExit -- normally not a problem because it's only thrown into a > generator when its close() method is called before it has terminated, to > cause any finally clauses it may have pending to execute. Then close() > swallows it back up. It's also called when the generator is > garbage-collected before it has terminated. A potential scenario where this > exception might cause a problem would be when a generator that's supposed > to perform something fundamental for asyncio somehow gets garbage-collected > (there was another thread here today that might or might not be due to this > happening). If this code were to catch BaseException, save it somewhere, > and then did another yield[from] or await, it might well be punished and > garbage-collected anyways. However I'm unclear in how such a coroutine > might end up being garbage-collected -- apparently nobody's waiting for it > any more? > Correct. In the cases that I'm dealing with the event loop is closed and I've even attempted to send cancel() to all the tasks I know about. So, GeneratorExit is thrown into a Task that no longer has a valid event loop. Ironically the errors I'm mostly dealing with are cases where code is doing naked except (catches BaseException) thusly getting invoked by the GeneratorExit and then assuming the event loop is in good standing, which it is not. So the subsequent calls to do event loop "stuff" fail with RuntimeError(...event loop closed...). The more I think about it, the more I'm inclined to think that application and/or protocol code that does wide exception handling needs to be prepared for scenarios like this. That might mean catching GeneratorExit separately and reraising it or doing event loop validation before blindly attempting to throw more work into an event loop that might be closed or otherwise in an invalid state. Something akin to... f = await open_async_res() try: await something_fragile() finally: if not asyncio.get_event_loop().is_closed(): await f.close() Can it generally be stated that context managers and finalizers in an event loop are burdened with validation of the event loop state (e.g. check that it's not closed), or should they be promised a valid event loop in all cases? I ask this somewhat rhetorically, as I believe there are certain scenarios where a valid event loop simply can't be guaranteed (as I tend to run into with my ephemeral use of event loops). If ephemeral use of event loops is considered an anti-pattern or just unsupported I'll kindly make due with the current status quo, as it's not too bad really. I admit these are largely corner cases and my ephemeral use of event loops is non-standard, even if I do think it's cool. ;) I guess you're right that the mind boggles. > > - User-defined subclasses of BaseException. Who knows what purpose these > have. Previous versions of asyncio used this to exit out of the event loop > when you call loop.stop() -- but it was used in a controlled way (the > stop() call doesn't raise the exception, it just schedules a callback that > raises it, and that callback will be called at a point where it's expected > and safe). However we just rewrote this code to use a different approach -- > it now just sets a flag that _run_once() checks. > > > All in all I have a feeling that catching BaseException instead of > Exception in asyncio isn't going to make a huge difference one way or > another. > > The problem you seem to be struggling with is more one of how to use the > loop's exception handler > It's an ancillary concern and I have code to work around it. It just feels needlessly complex because I'm forced to save exceptions and raise them outside the run_once/run_forever context to avoid it being gobbled up by default_exception_handler() (which is irrefutably called in call_exception_handler if an exception is raised by a custom exception handler). Regardless, I'm not blocked by the call_exception_handler behavior and I have a suitable workaround. No worries here, just some RFC. -- I noticed that your code uses undocumented loop instance variables (e.g. > loop._exception_handler). And possibly you're being bitten by the > KeyboardInterrupt issues I sketched above. Too much exception handling is > rarely a good thing. Maybe just let things bubble up instead of trying to > handle them locally? > Again, this is somewhat unrelated to the main issue I face but for clarification, this is more or less what my custom exception handler is trying to do; Save the exception and then raise it outside of call_exception_handler where it can actually get to the user. refs: https://github.com/mayfield/cellulario/blob/master/cellulario/iocell.py#L187 https://github.com/mayfield/cellulario/blob/master/cellulario/iocell.py#L218 Regarding `._exception_handler` I believe at one point the event loop used by an IOCell (which is ephemeral) was NOT created in that init_event_loop routine and I was trying to be a good citizen with what could have been a borrowed event loop. E.g. save the current exc handler, put mine in while we run, and restore the old one when we're done. The current behavior just makes a new event loop and I probably don't need to use the save/restore pattern per se. Also there wasn't a complementary get_exception_handler() method to go with set_exception_handler() and I didn't want to make assumptions about the event loop's setup. Ie. I wanted the code to work with any event loop subclass and event loops that may have had set_exception_handler() called previously by someone else. > > I'm still up for a patch to asyncio that makes its treatment of > BaseException more principled. Your hacked-up code ("swag" you call it -- > I've never heard that expression before) actually looks pretty reasonable > -- but can you live with it, or does it not address the issue you're having > in cellulario? > I can certainly live with it and I was surprised at how low touch it was. I agree that it's a good thing to support in general so I'm still supportive of handling BaseException in the event loop and Tasks. Unfortunately, it doesn't really address the issues I'm running into which are centered around suspended Tasks that get GC'd after their event loop is closed, thus triggering various finalizers and context managers who sometimes assume the event loop is in good order. I hope that run-on makes some sense. Thanks for your thoughtful attention to this Guido. Cheers, JM > > > On Tue, Dec 8, 2015 at 10:31 AM, Justin Mayfield <[email protected]> wrote: > >> Hi Guido, >> >> Yeah, I'm still investigating. I took a quick pass at just the >> BaseException handling. The swag can be seen on my fork, >> https://github.com/python/asyncio/compare/master...mayfield:master . >> >> After some minor tweaks tests pass on OSX in release mode for Python >> 3.5. The results are fairly tame actually, but the GeneratorExit at GC >> time issue is still making my brain hurt. >> >> My primary concern about call_exception_handler() is that it's perhaps >> too clever if (BaseException <= to_be_raised < Exception) is not longer >> sufficient to blast out of the event loop. While this may be a more >> common desire for exception handling in a system that continuously runs an >> event loop, a user may have to write a subclass or perform other >> workarounds to subvert the cleverness if they want an exception handling >> routine that is more than aesthetic (does more than logging). >> >> For example, this is some code I use for saving the exception >> https://github.com/mayfield/cellulario/blob/master/cellulario/iocell.py#L183 >> and >> then reraising it after a loop iteration, >> https://github.com/mayfield/cellulario/blob/master/cellulario/iocell.py#L218 >> . >> >> I certainly don't mean to undermine the thought that went into it, I just >> want to suggest that in light of potential changes to exception handling it >> may be worth discussion from stakeholders that know way more about it than >> I do. >> >> Cheers, >> JM >> >> PS: I'm a bit busy this week so I may not be able to look at this again >> till the weekend. >> >> On Mon, Dec 7, 2015 at 10:53 AM Guido van Rossum <[email protected]> >> wrote: >> >>> Heh, yeah, I may not have read your initial post in this thread too >>> closely... :-( >>> >>> I still think it should be possible to be more principled about how to >>> handle BaseException -- it seems about 50/50 whether it is treated the same >>> or differently. >>> >>> Regarding call_exception_handler(), I think it's intentional that it >>> doesn't let exceptions in the handler bubble out; in many call sites there >>> either is nothing else to be done anyway (e.g. it's called from various >>> __del__ methods) or the call site has an explicit assumption that it can do >>> more cleanup after it returns. >>> >>> This does remind me of an unfortunate thing: if KeyboardInterrupt is >>> caught while a __del__ method is running, *nothing* the code can do can >>> cause the KeyboardInterrupt to properly bubble up -- it is logged when >>> __del__ exits. But this is a Python problem, not an asyncio problem, and >>> what we decide to do in call_exception_handler() doesn't change this. >>> >>> Basically the handling of normal exceptions, when they arrive during >>> execution of a coroutine, is to transfer them to the Task wrapping the >>> coroutine (there always is one, possibly further up the stack). I think it >>> would be reasonable to try doing the same thing with BaseExceptions; or at >>> least to experiment and see what happens if we do that. Hopefully you're >>> still game for this experiment! >>> >>> On Sat, Dec 5, 2015 at 6:45 PM, Justin Mayfield <[email protected]> >>> wrote: >>> >>>> Your reply makes me think I have muddled two issues together. However, >>>> they may be linked so I'm moving forward with an experiment to unify >>>> exception handling such that BaseException handling is closer to >>>> Exception's handling. The consequences of such a change are going to be >>>> big from my initial investigation. >>>> >>>> The initial reason for my post was intended to help me understand >>>> behavior that felt erroneous. Namely a few cases where exception handlers >>>> ARE catching BaseException via naked except clauses and then making >>>> assumptions about the event loop state. I suppose I was trying to >>>> articulate that an argument could be made for adjusting these cases to be >>>> more like the current asyncio exception handling strategy (ie. don't catch >>>> BaseException). >>>> >>>> That being said, I tend to agree with your thinking of making asyncio >>>> robust enough to handle BaseException so that a user's code is not limited >>>> in their exception handling scope. Since these changes will likely affect >>>> and change the nature of the the corner cases in question, it's probably >>>> best to just focus BaseException handling first. >>>> >>>> One minor note about this; I notice that call_exception_handler() has >>>> a peculiar model where a user defined exception handler is guarded from >>>> raising its own exception; It calls the default exception handler instead, >>>> as described by this thread, >>>> https://groups.google.com/d/topic/python-tulip/XrcqtuXwMVc/discussion >>>> . I wonder if this deserves a second look in light of this discussion. >>>> My personal take is that exceptions from call_exception_handler are better >>>> served if they can blow through the stack. This could be their only >>>> opportunity to dirty-stop the event loop if BaseException is no longer >>>> special. It also seems to more obvious and more compatible with common >>>> save/restore patterns. >>>> >>>> JM >>>> >>>> On Friday, December 4, 2015 at 9:51:07 PM UTC-7, Guido van Rossum wrote: >>>>> >>>>> On Fri, Dec 4, 2015 at 11:39 AM, Justin Mayfield <[email protected]> >>>>> wrote: >>>>> >>>>>> Hi Guido, >>>>>> >>>>>> Yes, I'd be interested in that. >>>>>> >>>>> >>>>> This would be a great way to get your hands dirty! >>>>> >>>>> >>>>>> There are two things off the top of my head that I don't have a solid >>>>>> grasp on. >>>>>> >>>>>> 1. The effect of catching BaseException unilaterally and its affect >>>>>> on generator protocol. I'm not fluent enough in the Task mech to >>>>>> understand if this would break things. >>>>>> >>>>> >>>>> Hm, I don't think that BaseException is treated specially by the >>>>> generator protocol. Now, there might be some BaseExceptions where catching >>>>> them isn't going to help much. MemoryError comes to mind, but catching it >>>>> is unlikely to make things worse -- they are already pretty bad at that >>>>> point, and if it gets re-raised it'll likely eventually just bubble up the >>>>> stack until the program exits -- just as if you don't catch it. >>>>> >>>>> The bottom line is that catching exceptions isn't going to mess with >>>>> the interpreter's integrity, so whatever you do shouldn't overly worry >>>>> you. >>>>> >>>>> >>>>>> 2. Having seen the issue in asyncio and also aiohttp I assume there >>>>>> will be a never ending list of libraries that sometimes catch >>>>>> BaseException >>>>>> and some that do not. This may be unsolvable by trying to achieve >>>>>> consistency everywhere unless we can somehow convince an entire community >>>>>> that there is one, and only one, way to do exception handling in async >>>>>> code. That just seems like an impossible task to me but your influence >>>>>> may >>>>>> tell another story. >>>>>> >>>>> >>>>> I can't force people to change their code. And I don't think we should >>>>> worry about this -- if a library doesn't catch BaseException, it'll be >>>>> dealt with by asyncio itself, and that should be sufficient. If for a >>>>> particular use case it isn't, that is between the user and the library >>>>> author. Dealing with BaseException properly in asyncio should give both >>>>> users and library authors the confidence that this *can* be dealt with (as >>>>> opposed to the current situation where asyncio itself doesn't really try, >>>>> so why would other libraries bother). >>>>> >>>>> >>>>>> I'll do some experiments this weekend and report back. Please advise >>>>>> if you want an issue filed and I'll spend some time writing a test that >>>>>> fails. >>>>>> >>>>> >>>>> It's always better to track a specific issue in a tracker than in a >>>>> general mailing list. In this case I recommend using asyncio's own >>>>> tracker ( >>>>> https://github.com/python/asyncio) so we can refine the approach >>>>> without bothering the other CPython developers. >>>>> >>>>> Thanks again for volunteering to look into this! >>>>> >>>>> --Guido >>>>> >>>>> >>>>>> JM >>>>>> >>>>>> On Fri, Dec 4, 2015 at 12:08 PM Guido van Rossum <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Hi Justin, >>>>>>> >>>>>>> I think in the long run the event loop (and everything in asyncio) >>>>>>> needs to be much more careful with BaseExceptions. Their treatment is >>>>>>> uneven -- if we're writing "except Exception" they are not caught, but >>>>>>> when >>>>>>> we're writing "finally" or "except: ...; raise" they are caught. I think >>>>>>> the right thing to do is to tread BaseException the same way we're >>>>>>> treating >>>>>>> Exception, *except* that in cases where we catch it and continue running >>>>>>> the event loop we should probably still break out of the event loop >>>>>>> right >>>>>>> away. So a KeyboardError would still break out of the event loop >>>>>>> (without >>>>>>> executing the remaining scheduled callbacks; or perhaps after, similar >>>>>>> to >>>>>>> the way the new stop() behaves), but the event loop would be in a >>>>>>> consistent state -- if you were to restart the loop the KeyboardError >>>>>>> would >>>>>>> be delivered to the callback to which an Exception class would be >>>>>>> delivered. >>>>>>> >>>>>>> Hope this description makes sense. >>>>>>> >>>>>>> Maybe you're interested in improving asyncio by submitting a patch, >>>>>>> given that you're running into this problem and so have a motivation as >>>>>>> well as a use case that can drive the improvements? >>>>>>> >>>>>>> On Fri, Dec 4, 2015 at 12:54 AM, Justin Mayfield <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> I have an experimental library where I manage the lifecycle of >>>>>>>> event loops for purposes of allowing async routines to operate inside a >>>>>>>> classical generator (among other things). I create a new event loop >>>>>>>> for >>>>>>>> each use of my routine and upon completion or GC this event loop is >>>>>>>> closed. >>>>>>>> >>>>>>>> My recent testing has left me a bit perplexed as to the right way >>>>>>>> to deal with KeyboardInterrupt and possibly other BaseException >>>>>>>> exceptions >>>>>>>> that halt execution of an event loop. I've read a few posts on this >>>>>>>> list >>>>>>>> suggesting that the basic rule of thumb employed within asyncio >>>>>>>> itself, is >>>>>>>> to only catch Exception because BaseException is generally considered >>>>>>>> unrecoverable (or coro protocol as in GeneratorExit). However I seem >>>>>>>> to >>>>>>>> have found several instances where this rule is not followed and in the >>>>>>>> except clause, further calls are made that expect the event loop to be >>>>>>>> in >>>>>>>> good standing. >>>>>>>> >>>>>>>> Because I close the event loop when my routine is done/interrupted >>>>>>>> the eventual GeneratorExit exception thrown by the garbage collector >>>>>>>> triggers some cleanup/close routines in asyncio itself (as well as >>>>>>>> aiohttp) >>>>>>>> that eventually find themselves staring at RuntimeError('Event loop is >>>>>>>> closed'). >>>>>>>> >>>>>>>> My question to the community is if I'm witnessing bugs in these >>>>>>>> places where naked except clauses are used to perform cleanup/closing >>>>>>>> actions on an event loop or if I'm fundamentally doing something >>>>>>>> wrong. I >>>>>>>> hope it's not the latter as I've been pretty happy with the results of >>>>>>>> my >>>>>>>> experiment baring these corner cases. >>>>>>>> >>>>>>>> [Code in question is here: >>>>>>>> https://github.com/mayfield/cellulario/blob/master/cellulario/iocell.py >>>>>>>> ] >>>>>>>> >>>>>>>> Example traceback.. >>>>>>>> >>>>>>>> Traceback (most recent call last): >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 692, in _create_connection_transport >>>>>>>> >>>>>>>> yield from waiter >>>>>>>> >>>>>>>> GeneratorExit >>>>>>>> >>>>>>>> >>>>>>>> During handling of the above exception, another exception occurred: >>>>>>>> >>>>>>>> >>>>>>>> Traceback (most recent call last): >>>>>>>> >>>>>>>> File >>>>>>>> "/Users/mayfield/project/syndicate/syndicate/adapters/async.py", line >>>>>>>> 74, >>>>>>>> in request >>>>>>>> >>>>>>>> result = yield from asyncio.wait_for(r, timeout, loop=self.loop) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/tasks.py", >>>>>>>> line 359, in wait_for >>>>>>>> >>>>>>>> return (yield from fut) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/lib/python3.5/site-packages/aiohttp-0.18.3-py3.5-macosx-10.10-x86_64.egg/aiohttp/client.py", >>>>>>>> line 456, in __iter__ >>>>>>>> >>>>>>>> resp = yield from self._coro >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/lib/python3.5/site-packages/aiohttp-0.18.3-py3.5-macosx-10.10-x86_64.egg/aiohttp/client.py", >>>>>>>> line 173, in _request >>>>>>>> >>>>>>>> conn = yield from self._connector.connect(req) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/lib/python3.5/site-packages/aiohttp-0.18.3-py3.5-macosx-10.10-x86_64.egg/aiohttp/connector.py", >>>>>>>> line 289, in connect >>>>>>>> >>>>>>>> transport, proto = yield from self._create_connection(req) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/lib/python3.5/site-packages/aiohttp-0.18.3-py3.5-macosx-10.10-x86_64.egg/aiohttp/connector.py", >>>>>>>> line 557, in _create_connection >>>>>>>> >>>>>>>> server_hostname=hinfo['hostname'] if sslcontext else None) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 669, in create_connection >>>>>>>> >>>>>>>> sock, protocol_factory, ssl, server_hostname) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 694, in _create_connection_transport >>>>>>>> >>>>>>>> transport.close() >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/selector_events.py", >>>>>>>> line 566, in close >>>>>>>> >>>>>>>> self._loop.call_soon(self._call_connection_lost, None) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 453, in call_soon >>>>>>>> >>>>>>>> handle = self._call_soon(callback, args) >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 462, in _call_soon >>>>>>>> >>>>>>>> self._check_closed() >>>>>>>> >>>>>>>> File >>>>>>>> "/usr/local/Cellar/python3/3.5.0/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", >>>>>>>> line 289, in _check_closed >>>>>>>> >>>>>>>> raise RuntimeError('Event loop is closed') >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> --Guido van Rossum (python.org/~guido) >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> --Guido van Rossum (python.org/~guido) >>>>> >>>> >>> >>> >>> -- >>> --Guido van Rossum (python.org/~guido) >>> >> > > > -- > --Guido van Rossum (python.org/~guido) >
