This project is read-only.

Problem with _exceptionPending implementation

Jul 23, 2014 at 9:58 PM
If there is an error writing to the underlying stream, ZipOutputStream's Dispose() method leaves the underlying stream open. The cause of this is the _exceptionPending logic.

In ZipOutputStream.cs there is a variable called _exceptionPending with a comment that says:
// When ZipOutputStream is employed within a using clause, which
// is the typical scenario, and an exception is thrown within
// the scope of the using, Close()/Dispose() is invoked
// implicitly before processing the initial exception.  In that
// case, _exceptionPending is true, and we don't want to try to
// write anything in the Close/Dispose logic.  Doing so can
// cause additional exceptions that mask the original one. So,
// the _exceptionPending flag is used to track that, and to
// allow the original exception to be propagated to the
// application without extra "noise."
That makes sense. But current implementation using _exceptionPending does not actually address this problem. Also, the current implementation leaks the underlying file stream in that case.

So regarding the first problem:
_exceptionPending is only set to true when the ZipOutputStream code throws an exception. It does not get set if the underlying stream throws an exception. Oops! For example, if the volume is dismounted, disk space fills up, etc.

In that case, _exceptionPending = false so the Dispose() tries to do all the usual work, and thus throws an exception. That exception not only means it "masks" the original exception as described in the comment, but it also never gets to the Dispose() call on the underlying stream!

And the second problem:
If _exceptionPending = true, it skips the entire Dispose() method, except for setting _disposed = true. This means the underlying stream is not closed. In our application, it meant the stream was left open until garbage collection closed it.

I think the original idea in the code is correct, but it did not go far enough.

The Framework Design Guidelines (2nd ed) has this as (§9.4.1):
AVOID throwing an exception from within Dispose(bool) except under critical situations where the containing process has been corrupted (leaks, inconsistent shared state, etc.).
I believe that the correct fix is to change the Dispose() method to simply not throw exceptions. Here is what our team did:
1) Put a try/catch block around the calls that flush the file and swallow whatever exception happens that. So even if those calls throw an exception, the underlying stream is still closed.
2) Dispose() the underlying stream regardless of the setting of _exceptionPending. Also, put a try/catch around that in case the underlying stream gets an exception.

We did this in our application and it solved our problem.

My only question is this: At that point, _exceptionPending serves no purpose. It still could be used to prevent it from even trying to flush to disk. But with a try/catch + swallow there, I see no benefit to it.

I would like some feedback before I try to submit a patch. (I've never done that to CodePlex before).