On Mon, Jan 25, 2010 at 02:22:34PM -0600, Shawn Walker wrote: > On 01/25/10 01:51 PM, [email protected] wrote: > >On Sat, Jan 23, 2010 at 04:26:03PM -0600, Shawn Walker wrote: > >>>http://cr.opensolaris.org/~johansen/webrev-3720-3/ > ... > >> line 1772: does this need to be wrapped with try:/finally: > >>activity_lock.release() in case enable_cancel fails? > > > >This is a generator function. I didn't see a tidy way to wrap this in > >another function with try/finally and have things work correctly. > > It doesn't matter if it is a generator function. Python 2.5+ > guarantees the finally bock will be executed: > > http://docs.python.org/whatsnew/2.5.html > > "...This last chance means that try...finally statements in > generators can now be guaranteed to work; the finally clause will > now always get a chance to run." > > Or did I misunderstand the concern here?
That's part of the concern; however, the other part of the concern is that there's no way to add another level of indendation to remote_search without making the function unreadable. My other concern was how to appropriately wrap the remote_search, a function that calls yield, with another function that has the try/finally block and still retains the functionality of a generator. I had thought that if I tried to iterate over the response from remote_search and yield that instead, I would pull all of the search results into memory. > >Enable_cancel can't fail unless there's an assertion failure. My > >understanding is that these should propogate to the top and cause a > >traceback. I don't think we need to worry about that particular case > >here. > > Yes, they may cause a traceback, but in the case of the GUI, the > client won't exit after that traceback, so I believe the lock still > needs to be released in relevant exception scenarios. That's broken. The whole point of having an assertion failure is to failfast and exit. > >>Likewise, it looks like if an exception happens in several spots in > >>here that the activity lock may not be released. > > > >Which spots? I went through this code pretty carefully to catch the > >places where it looked like there might be an uncaught exception. If > >you think I've missed one or more, would you please provide specifics? > > line 1775, 1779-1780 Ok. I'll take a look at these. > 1821 This doesn't re-raise the exception once it's caught. This ought to be safe. > and 1569 (the cancel state callback could raise an exception) seem > like candidates. There may be others related. Does the API permit the cancel state callback to raise an exception? IMO, it doesn't since no other part of the code is written to cope with that failing. We should just explicitly document that this callback must not return an exception and be done with it. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
