Code review comment for lp:~jameinel/launchpad/twisted-close-on-failure

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/18/2011 10:07 AM, Benji York wrote:
> On Fri, Feb 18, 2011 at 11:03 AM, John A Meinel <email address hidden> wrote:
>> The FAILURE check was removed because the code we called already
>> checked for it.
> [...]
>> So I was just removing duplication.
>
> Ah! That makes sense.
>
>> Yes, we no longer close the fd, because the
>> ProcessWriter/ProcessReader will close it. I can switch the comment if
>> you find it easier to understand.
>
> I do find it clearer. That being said, I eventually figured it out so
> the original comment can't be all bad.

Done.

>
>> For the cleanup code... we could suppress all exceptions at that
>> point, I guess. Its a little ugly, but that may not be preventable.
>> FWIW, bzr has cleanup code for this sort of thing, except it always
>> runs the cleanups (but suppresses failures if there is a current
>> exception running.)
>
> Right, the central question is: should a failure of the cleanup code
> cause an exception to bubble up or should it be silently ignored. It's
> possible that that failure could cause file handles to be leaked, which
> is what you're trying hard to prevent. On the other hand, a rare
> failure when cleaning up probably shouldn't cause the entire process to
> end, so ignoring exceptions from cleanup code seems like the lesser
> evil.

I changed it to trap the original exception using sys.exc_info(), then
iterate the cleanups, and if there is another exception just call log.err()
Then raise the original exception again.

Which seemed a reasonable tradeoff.

Committed and pushed.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1em8QACgkQJdeBCYSNAAOipACeJJOl3BSzYoRelLS2JrSGt9YU
4xgAnR/lWKNZutRqJB4enGW9Q/WOALQU
=+lBJ
-----END PGP SIGNATURE-----