On 01/25/10 03:26 PM, [email protected] wrote:
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.
Yes, the indentation thing is why I ended up wrapping functions with
functions in some cases for the image locking work I did. In some
cases, that actually simplified the code since I was able to remove
multiple exception handling cases and simply put them once at the
top-level in a wrapper.
As for the generator bit, if the wrapper function does something like:
try:
return self.__generator_func()
except ...:
...
finally:
....
...it will be fine. There's no real difference between returning a
generator (as shown above) or calling the generator directly.
I might still be missing something here though.
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.
We may wish to document that in the docstring for pkg.client.api. Since
AssertionError inherits from the Exception base class, that would mean
all clients need a special handler for AssertionErrors.
I'm also not certain that the GUI client should just exit right away,
although at the very least it should re-create its existing API objects.
The client spontaneously existing when an unexpected failure occurs
(even if the resulting error is displayed) seems like a recipe for user
frustration in the GUI case.
But I'll defer to whatever the group opinion is here.
...
1821
This doesn't re-raise the exception once it's caught. This ought to be
safe.
The thought was that the origin may be invalid, but I suppose we should
assume that user input has been validated first.
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.
That's probably fine.
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
