SpreadsheetDocument.Dispose() throws System.ObjectDisposedException

[sszymons]

Describe the bug
SpreadsheetDocument.Dispose() throws System.ObjectDisposedException: Cannot access a closed Stream.

Screenshots

To Reproduce

         Stream stream = new MemoryStream();
         var spreadsheetDocument = SpreadsheetDocument.Create(stream, SpreadsheetDocumentType.Workbook);
         spreadsheetDocument.Save();
         
         stream.Dispose();
         spreadsheetDocument.Dispose();

Observed behavior
Exception is thrown.

System.ObjectDisposedException : Cannot access a closed Stream. at System.IO.MemoryStream.Seek(Int64 offset, SeekOrigin loc) at System.IO.Compression.ZipArchive.WriteFile() at System.IO.Compression.ZipArchive.Dispose(Boolean disposing) at System.IO.Compression.ZipArchive.Dispose() at System.IO.Packaging.ZipPackage.Dispose(Boolean disposing) at System.IO.Packaging.Package.System.IDisposable.Dispose() at System.IO.Packaging.Package.Close() at DocumentFormat.OpenXml.Features.StreamPackageFeature.Dispose(Boolean disposing) at DocumentFormat.OpenXml.Features.StreamPackageFeature.Dispose() at DocumentFormat.OpenXml.Packaging.PackageFeatureCollection.DocumentFormat.OpenXml.Features.IContainerDisposableFeature.Dispose() at DocumentFormat.OpenXml.Packaging.OpenXmlPackage.Dispose(Boolean disposing) at DocumentFormat.OpenXml.Packaging.OpenXmlPackage.Dispose()

Expected behavior
Should not throw exception, according to:https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1065

A System.IDisposable.Dispose method should not throw an exception. Dispose is often called as part of the cleanup logic in a finally clause. Therefore, explicitly throwing an exception from Dispose forces the user to add exception handling inside the finally clause.

Desktop (please complete the following information):

• OS: MasOS 13.5
• Office version: N/A
• .NET Target: .NET 6
• DocumentFormat.OpenXml Version: 3.0.2

[m-gallesio]

Have you considered moving from manual .Dispose() to using statements?

[sszymons]

Have you considered moving from manual .Dispose() to using statements?

Of course. The original implementation uses a "using" statement. The example is just simplified.

[edwardneal]

This is because of behaviour in ZipArchive. This behaviour needs to change, but by way of explanation: although calling SpreadsheetDocument.Save will persist the relevant parts into the ZipArchive, the ZipArchive won't be persisted into its stream. There's a PR to add ZipArchive.Flush to do this, dotnet/runtime#24149.

ZipArchive won't flush its content into the stream until the ZipArchive instance is disposed of. This happens when the Package is disposed of, which in turn happens when the SpreadsheetDocument is disposed of.

In the sample, you're disposing of the stream, then the SpreadsheetDocument, so when the ZipArchive tries to flush its content into the stream, it throws the ObjectDisposedException you've found. If you dispose of the SpreadsheetDocument before the stream, I think it'll work.

The complete fix would be for ZipArchive to implement Flush, then for ZipPackage to use this so that the Dispose behaviour can be reviewed (at the very least, so it doesn't throw if you Dispose straight after calling SpreadsheetDocument.Save.) This needs to wait for the PR linked above though.

[sszymons]

This is because of behaviour in ZipArchive. This behaviour needs to change, but by way of explanation: although calling SpreadsheetDocument.Save will persist the relevant parts into the ZipArchive, the ZipArchive won't be persisted into its stream. There's a PR to add ZipArchive.Flush to do this, dotnet/runtime#24149.

ZipArchive won't flush its content into the stream until the ZipArchive instance is disposed of. This happens when the Package is disposed of, which in turn happens when the SpreadsheetDocument is disposed of.

In the sample, you're disposing of the stream, then the SpreadsheetDocument, so when the ZipArchive tries to flush its content into the stream, it throws the ObjectDisposedException you've found. If you dispose of the SpreadsheetDocument before the stream, I think it'll work.

The complete fix would be for ZipArchive to implement Flush, then for ZipPackage to use this so that the Dispose behaviour can be reviewed (at the very least, so it doesn't throw if you Dispose straight after calling SpreadsheetDocument.Save.) This needs to wait for the PR linked above though.

I got it. Thank you! It looks like we cannot do anything in Open XML SDK implementation. Can this issue stay open?

[twsouthwick]

I don't think this is the ziparchive - you have the dispose statements the wrong direction. This is part of why using statements are so much better. Try:

Stream stream = new MemoryStream();
 var spreadsheetDocument = SpreadsheetDocument.Create(stream, SpreadsheetDocumentType.Workbook);
 spreadsheetDocument.Save();
 
- stream.Dispose();
 spreadsheetDocument.Dispose();
+ stream.Dispose();

The ziparchive may play into it, but disposing out of order like this is guessing at what the implementation should be doing. Even if "spreadsheetDocument.Save()` flushed everything, the dispose may do additional cleanup and map expect the underlying package/stream to still be available.

[twsouthwick]

I'm going to close this as it's by design here. Feel free to reopen if this doesn't resolve it.