Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async dispose support to ZipArchive #1560

Closed
MarcusWichelmann opened this issue Oct 25, 2019 · 10 comments · May be fixed by manandre/runtime#1
Closed

Add async dispose support to ZipArchive #1560

MarcusWichelmann opened this issue Oct 25, 2019 · 10 comments · May be fixed by manandre/runtime#1

Comments

@MarcusWichelmann
Copy link

In my application I am creating a ZipArchive that writes directly to the ASP.Net Core body stream, like that:

using var zipArchive = new ZipArchive(Response.Body, ZipArchiveMode.Create, true);

Asynchronously writing file contents to that archive works fine, but as soon as the writing is finished and the ZipArchive gets disposed, the ZipArchive tries to synchronously write some more data to the stream. This results in an exception, because Kestrel, by default, doesn't allow synchronous writes anymore:

System.InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpResponseStream.Write(Byte[] buffer, Int32 offset, Int32 count)
   at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionBody.Write(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.Compression.PositionPreservingWriteOnlyStreamWrapper.Write(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.BinaryWriter.Write(UInt32 value)
   at System.IO.Compression.ZipArchiveEntry.WriteCentralDirectoryFileHeader()
   at System.IO.Compression.ZipArchive.WriteFile()
   at System.IO.Compression.ZipArchive.Dispose(Boolean disposing)
   at System.IO.Compression.ZipArchive.Dispose()

Enabling async disposal for this class and then doing the necessary writes asynchronously would probably resolve this issue.

@vtchalkov
Copy link

looks like duplicate of https://github.com/dotnet/corefx/issues/5681

@davidfowl
Copy link
Member

davidfowl commented Oct 26, 2019

Does it work if you use FlushAsync before disposing? There is no flush API...

@carlossanlop
Copy link
Member

Triage:
@vtchalkov the ZipFile issue is not a duplicate.
This looks like a legitimate scenario. Next step would be to add a PR to implement it and add DisposeAsync.

@manandre
Copy link
Contributor

This needs first to add async methods to BinaryWriter via dotnet/corefx#8382

@manandre
Copy link
Contributor

Test running successfully with manandre#1 👍

[Fact]
public static async Task DiposeAsyncCallsWriteAsyncOnly()
{
    MemoryStream ms = new MemoryStream();
    CallTrackingStream trackingStream = new CallTrackingStream(ms);

    await using (ZipArchive archive = new ZipArchive(trackingStream, ZipArchiveMode.Create))
    {
        archive.CreateEntry("hey");
    }

    Assert.Equal(0, trackingStream.TimesCalled("Write"));
    Assert.NotEqual(0, trackingStream.TimesCalled("WriteAsync"));
}

@carlossanlop carlossanlop transferred this issue from dotnet/corefx Jan 9, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO.Compression untriaged New issue has not been triaged by the area owner labels Jan 9, 2020
@carlossanlop carlossanlop removed the untriaged New issue has not been triaged by the area owner label Jan 9, 2020
@carlossanlop carlossanlop added this to the 5.0 milestone Jan 9, 2020
@carlossanlop carlossanlop modified the milestones: 5.0.0, Future Jun 18, 2020
@madelson
Copy link
Contributor

This would be beneficial for us as well!

@anranruye
Copy link

Is there any progress?

@adamsitnik
Copy link
Member

Is there any progress?

We haven't made any progress recently. I am going to move it to 7.0 milestone to make sure we discuss including it in .NET 7.0

@adamsitnik adamsitnik modified the milestones: Future, 7.0.0 May 17, 2022
@anranruye
Copy link

A workaround for this scenario is to use Response.BodyWriter.AsStream() instead.

@carlossanlop
Copy link
Member

The discussion in this issue has been very helpful. I think we should implement all ZipFile and ZipArchive async APIs at the same time. For that reason, I'll close this issue in favor of the oldest and most generic issue (#1541), and will add a comment there to make sure to come back here to look at the comments here.

@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants