Zip async implementation #114421

carlossanlop · 2025-04-09T02:58:19Z

Fixes #1541

Approved APIs: #1541 (comment)

Each commit is small to help with reviewing.

@edwardneal - our changes will conflict. Ideally your PR should get merged first and I would update mine afterwards.

edwardneal

Thanks @carlossanlop. If time's limited, could we look at #113306 before #114256 though please?

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Async.cs

carlossanlop · 2025-04-09T17:00:11Z

If time's limited, could we look at #113306 before #114256 though please?

Looking at them today. I think we can get them all merged before the next snap.

I wanted to ask you @edwardneal: Are you on discord by any chance? Maybe we could communicate faster via chat. Feel free to add me. My username is chayote_jarocho .

ericstj

Had a brief look - try and find opportunities for sharing non-trivial logic between sync / async.

@edwardneal's suggestion is a good one but also consider factoring synchronous chunks of logic in to shared methods.

If you cannot share the code then consider co-locating it if it must be maintained in parallel.

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.Async.cs

...O.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchive.Create.Async.cs

...ression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Async.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs

carlossanlop · 2025-04-10T23:03:03Z

Marked as ready for review because I pushed a ton of tests. I will keep adding some more.

src/libraries/System.IO.Compression.ZipFile/ref/System.IO.Compression.ZipFile.cs

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.Async.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Sync.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Async.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs

adamsitnik

Overall it looks good, I had few quesitons and found only some nits.

I have not compared the sync vs async implementations and since this code contains a lot of if blocks it would make me feel better about approving it if you could add some fuzz tests for it.

A simple fuzz test could look like this:

Take the bytes generated by the fuzzer, write them to file.
Compress this file with sync API.
Compress this file with async API.
Ensure both produced the same output.
Decompress the output with sync API.
Decompress the output with async API.
Ensure the result is the same as input.

BTW I am really excited to see those APIs being added in .NET 10. I know users have been waiting for this moment for a long time.

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.Async.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Sync.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_LargeFiles.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs

carlossanlop · 2025-04-15T15:28:43Z

Need to fix the merge conflicts with f93aa8a , 1b090a9 and 44c08b8

edwardneal

I've got one final set of comments - mostly optimisations around memory allocation. All of them could be changed in subsequent PRs though, the logic itself looks sound.

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Async.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Async.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs

carlossanlop · 2025-04-16T07:45:19Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2025-04-16T07:45:28Z

Azure Pipelines successfully started running 1 pipeline(s).

carlossanlop · 2025-04-16T21:03:37Z

@adamsitnik I added fuzz tests.

carlossanlop · 2025-04-16T23:25:42Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2025-04-16T23:25:52Z

Azure Pipelines successfully started running 1 pipeline(s).

…6,384).

…s in ZipFile and ZipFileExtensions. Remove redundant switch case.

… null.

…o the right location, remove the unnecessary suppression text.

… calling sync APIs, and viceversa.

…alling sync APIs and viceversa.

…r the system. Keep one sync and another async instead.

…asses.

…ome cases due to mixing of static readonly and consts in some block types by setting Signature value to the fixed number 4, which will most likely never change.

… running async they fail with 'Field marshaling is not supported by Invoke: async'.

…f having one.

carlossanlop · 2025-04-17T10:00:09Z

I investigated the failures, they are happening in wasm due to async code not being able to run nicely in single threaded environments. The error is happening in all async tests and says "PlatformNotSupportedException: Cannot wait on monitors for this runtime". I opened a KnownBuildError issue to track this, will work on fixing it tomorrow: #114769

Aside from that, all other failures have been fixed, so this is good to merge.

adamsitnik

LGTM, thank you for addressing my feedback @carlossanlop !

src/libraries/Common/tests/System/IO/Compression/NoAsyncCallsStream.cs

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

adamsitnik · 2025-04-17T07:55:55Z

src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipArchiveFuzzer.cs

+
+    public void FuzzTarget(ReadOnlySpan<byte> bytes)
+    {
+


Suggested change

adamsitnik · 2025-04-17T07:58:08Z

src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipArchiveFuzzer.cs

+
+            Task.WaitAll(sync_test, async_test);
+        }
+        catch (Exception) { }


Would it be possible to catch only the exceptions that we expect here? For example this is how we do it for NRBF:

runtime/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/NrbfDecoderFuzzer.cs

Lines 56 to 61 in b281500

catch (SerializationException) { /* Reading from the stream encountered invalid NRBF data.*/ }

catch (NotSupportedException) { /* Reading from the stream encountered unsupported records */ }

catch (DecoderFallbackException) { /* Reading from the stream encountered an invalid UTF8 sequence. */ }

catch (EndOfStreamException) { /* The end of the stream was reached before reading SerializationRecordType.MessageEnd record. */ }

catch (IOException) { /* An I/O error occurred. */ }

It has helped me to find that the library was throwing undocumented exceptions (part of threat model)

I think there are more things I can improve here besides what you suggest. I'll include such changes in the follow up PR.

carlossanlop added the area-System.IO.Compression label Apr 9, 2025

carlossanlop requested review from stephentoub, ViktorHofer, ericstj and a team April 9, 2025 02:58

carlossanlop self-assigned this Apr 9, 2025

ghost added the new-api-needs-documentation label Apr 9, 2025

This comment was marked as duplicate.

Sign in to view

This comment was marked as resolved.

Sign in to view

edwardneal reviewed Apr 9, 2025

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.Async.cs Show resolved Hide resolved

ericstj reviewed Apr 10, 2025

View reviewed changes

carlossanlop commented Apr 10, 2025

View reviewed changes

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs Show resolved Hide resolved

carlossanlop marked this pull request as ready for review April 10, 2025 23:02

build-analysis bot mentioned this pull request Apr 11, 2025

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

jozkee reviewed Apr 11, 2025

View reviewed changes

edwardneal reviewed Apr 15, 2025

View reviewed changes

adamsitnik reviewed Apr 15, 2025

View reviewed changes

carlossanlop force-pushed the ZipAsyncImplementation branch from e7a3a28 to 5ffb3cb Compare April 16, 2025 01:34

edwardneal reviewed Apr 16, 2025

View reviewed changes

ViktorHofer approved these changes Apr 16, 2025

View reviewed changes

carlossanlop added this to the 10.0.0 milestone Apr 16, 2025

build-analysis bot mentioned this pull request Apr 17, 2025

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts #114241

Closed

carlossanlop added 17 commits April 16, 2025 22:34

src: Add missed overloaded method in ZipFileExtensions.

5e65586

src: Share some more code in ZipFileExtensions, add missing docs

093238e

src: Increase bufferSize in ZipFileExtensions FileStream to 0x4000 (1…

2148fd5

…6,384).

src: Avoid nullable archiveStream in new constructor.

4eefc87

src: Remove default cancellation token in private and internal method…

7efc9ac

…s in ZipFile and ZipFileExtensions. Remove redundant switch case.

src: DceideArchiveStream should throw expected exception if stream is…

722de82

… null.

src: Simplify GetFileStreamForOpen switch.

7a9f252

src: Share the file stream buffer size. Move the filestream comment t…

82dbf3b

…o the right location, remove the unnecessary suppression text.

src: ZipBlocks put together two checks that return false.

59484c0

src: Simplify return values in async code and shared code.

c629fa3

Fuzz test for sync and async

fda1978

tests: Add special tests that confirm that the new async APIs are not…

f3566a6

… calling sync APIs, and viceversa.

src: Fix bugs found with the tests that check if async APIs are not c…

d4509c5

…alling sync APIs and viceversa.

src: Fix test method rename build failure

0ad6c39

tests: Remove two of the four outerloop tests as they're too heavy fo…

a87d789

…r the system. Keep one sync and another async instead.

src: Implement the missing async methods in internal helper stream cl…

6afdb55

…asses.

src: Fix NullReferenceException and static initialization issues in s…

0b6e6fe

…ome cases due to mixing of static readonly and consts in some block types by setting Signature value to the fixed number 4, which will most likely never change.

carlossanlop force-pushed the ZipAsyncImplementation branch from 93d9799 to 0b6e6fe Compare April 17, 2025 06:11

carlossanlop added 2 commits April 17, 2025 00:07

tests: revert zipfile unix tests as they use remote executor and when…

7458a57

… running async they fail with 'Field marshaling is not supported by Invoke: async'.

tests: Split the ExtractOutOfRoot tests into sync and async instead o…

cb1e358

…f having one.

carlossanlop mentioned this pull request Apr 17, 2025

Exclude System.IO.Compression async tests from wasm #114769

Closed

carlossanlop merged commit b40903f into dotnet:main Apr 17, 2025
77 of 85 checks passed

carlossanlop deleted the ZipAsyncImplementation branch April 17, 2025 10:00

adamsitnik approved these changes Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zip async implementation #114421

Zip async implementation #114421

carlossanlop commented Apr 9, 2025 •

edited

Loading

This comment was marked as duplicate.

This comment was marked as resolved.

edwardneal left a comment

carlossanlop commented Apr 9, 2025

ericstj left a comment

carlossanlop commented Apr 10, 2025

adamsitnik left a comment

carlossanlop commented Apr 15, 2025 •

edited

Loading

edwardneal left a comment

carlossanlop commented Apr 16, 2025

azure-pipelines bot commented Apr 16, 2025

carlossanlop commented Apr 16, 2025

carlossanlop commented Apr 16, 2025

azure-pipelines bot commented Apr 16, 2025

carlossanlop commented Apr 17, 2025

adamsitnik left a comment

adamsitnik Apr 17, 2025

adamsitnik Apr 17, 2025

carlossanlop Apr 17, 2025

	catch (SerializationException) { /* Reading from the stream encountered invalid NRBF data.*/ }
	catch (NotSupportedException) { /* Reading from the stream encountered unsupported records */ }
	catch (DecoderFallbackException) { /* Reading from the stream encountered an invalid UTF8 sequence. */ }
	catch (EndOfStreamException) { /* The end of the stream was reached before reading SerializationRecordType.MessageEnd record. */ }
	catch (IOException) { /* An I/O error occurred. */ }

Zip async implementation #114421

Zip async implementation #114421

Conversation

carlossanlop commented Apr 9, 2025 • edited Loading

This comment was marked as duplicate.

This comment was marked as resolved.

edwardneal left a comment

Choose a reason for hiding this comment

carlossanlop commented Apr 9, 2025

ericstj left a comment

Choose a reason for hiding this comment

carlossanlop commented Apr 10, 2025

adamsitnik left a comment

Choose a reason for hiding this comment

carlossanlop commented Apr 15, 2025 • edited Loading

edwardneal left a comment

Choose a reason for hiding this comment

carlossanlop commented Apr 16, 2025

azure-pipelines bot commented Apr 16, 2025

carlossanlop commented Apr 16, 2025

carlossanlop commented Apr 16, 2025

azure-pipelines bot commented Apr 16, 2025

carlossanlop commented Apr 17, 2025

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik Apr 17, 2025

Choose a reason for hiding this comment

adamsitnik Apr 17, 2025

Choose a reason for hiding this comment

carlossanlop Apr 17, 2025

Choose a reason for hiding this comment

carlossanlop commented Apr 9, 2025 •

edited

Loading

carlossanlop commented Apr 15, 2025 •

edited

Loading