Refactor sync/async implementations by using Adapter pattern by alinpahontu2912 · Pull Request #129282 · dotnet/runtime

alinpahontu2912 · 2026-06-11T08:56:37Z

dotnet-policy-service · 2026-06-11T08:58:04Z

Tagging subscribers to this area: @dotnet/area-system-formats-tar
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR refactors System.Formats.Tar to deduplicate synchronous and asynchronous read/write implementations by introducing a static-abstract IReadWriteAdapter and routing both sync/async public APIs through shared generic “CoreAsync” methods.

Changes:

Introduces IReadWriteAdapter with SyncReadWriteAdapter / AsyncReadWriteAdapter implementations to unify stream operations.
Reworks TarReader / TarWriter to funnel sync and async APIs through shared generic *CoreAsync<TAdapter> implementations.
Updates shared helpers and header read/write logic (TarHelpers, TarHeader.Read, TarHeader.Write) to use the adapter-based core methods.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs	Routes sync/async write APIs through a unified adapter-based core implementation.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReadWriteAdapter.cs	Adds the sync/async adapter abstraction (static-abstract interface + implementations).
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs	Routes sync/async read APIs through a unified adapter-based core implementation.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs	Refactors stream-advance/copy helpers into generic core methods using adapters.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs	Unifies header write logic behind adapter-based core methods.
src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj	Includes the new `TarReadWriteAdapter.cs` in compilation.

+        internal static async ValueTask<TarHeader?> TryGetNextHeaderCoreAsync<TAdapter>(Stream archiveStream, bool copyData, TarEntryFormat initialFormat, bool processDataBlock, CancellationToken cancellationToken)
+            where TAdapter : IReadWriteAdapter
        {
-            // The four supported formats have a header that fits in the default record size
-            Span<byte> buffer = stackalloc byte[TarHelpers.RecordSize];
-
-            archiveStream.ReadExactly(buffer);
-
-            TarHeader? header = TryReadAttributes(initialFormat, buffer, archiveStream);
-            if (header != null && processDataBlock)
-            {
-                header.ProcessDataBlock(archiveStream, copyData);
-            }
-
-            return header;
-        }
-
-        // Asynchronously attempts read all the fields of the next header.
-        // Throws if end of stream is reached or if any data type conversion fails.
-        // Returns true if all the attributes were read successfully, false otherwise.
-        internal static async ValueTask<TarHeader?> TryGetNextHeaderAsync(Stream archiveStream, bool copyData, TarEntryFormat initialFormat, bool processDataBlock, CancellationToken cancellationToken)
-        {
-            cancellationToken.ThrowIfCancellationRequested();
-
            // The four supported formats have a header that fits in the default record size
            byte[] rented = ArrayPool<byte>.Shared.Rent(minimumLength: TarHelpers.RecordSize);
            Memory<byte> buffer = rented.AsMemory(0, TarHelpers.RecordSize); // minimumLength means the array could've been larger


teo-tsirpanis · 2026-06-11T14:11:24Z

+        public static ValueTask<int> ReadAsync(Stream stream, Memory<byte> buffer, CancellationToken cancellationToken) =>
+            stream.ReadAsync(buffer, cancellationToken);
+
+        public static ValueTask ReadExactlyAsync(Stream stream, Memory<byte> buffer, CancellationToken cancellationToken) =>
+            stream.ReadExactlyAsync(buffer, cancellationToken);
+
+        public static ValueTask WriteAsync(Stream stream, ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken) =>
+            stream.WriteAsync(buffer, cancellationToken);
+
+        public static ValueTask CopyToAsync(Stream source, Stream destination, CancellationToken cancellationToken) =>
+            new ValueTask(source.CopyToAsync(destination, cancellationToken));
+
+        public static ValueTask AdvanceToEndAsync(SubReadStream stream, CancellationToken cancellationToken) =>
+            stream.AdvanceToEndAsync(cancellationToken);
+
+        public static ValueTask DisposeAsync(Stream stream) => stream.DisposeAsync();


Add awaits to help runtime async? Or are they not necessary?

There is work in progress to make forwards like this work well in runtime async.

Note that await changes observable behaviors: It wraps exceptions in the Task and it saves/restores async context.

rzikm

There seem to be some private/internal methods that are not converted (i commented on them), if we call them only from a single place, I think we should remove them

rzikm · 2026-06-11T15:47:44Z

+    // SyncReadWriteAdapter and asserts the returned ValueTask is already completed;
+    // the async entry point passes AsyncReadWriteAdapter. Mirrors the IReadWriteAdapter
+    // pattern used by SslStream / SmtpClient (see src/libraries/Common/src/System/Net/ReadWriteAdapter.cs).
+    internal interface IReadWriteAdapter


Do we need to duplicate the adapter? can't we reuse/enhance the existing adapter we use in SslStream?

I think that is possible, probably need to update the csproj file

Is it a good idea to add a reference the the netwroking project here though ? I think the adapters are also slightly different, since tar doesn't need the waitasync, readatleast and ssl doesn't need advancetoend/readexactly from what I can see, so maybe it's not worth combining them ? Or maybe we should create something super generic in a common project and reuse that adapter everywhere?

rzikm · 2026-06-11T16:00:15Z

        internal static void AdvanceStream(Stream archiveStream, long bytesToDiscard)
        {
-            if (archiveStream.CanSeek)
-            {
-                archiveStream.Position += bytesToDiscard;
-            }
-            else if (bytesToDiscard > 0)
-            {
-                byte[] buffer = ArrayPool<byte>.Shared.Rent(minimumLength: (int)Math.Min(MaxBufferLength, bytesToDiscard));
-                while (bytesToDiscard > 0)
-                {
-                    int currentLengthToRead = (int)Math.Min(MaxBufferLength, bytesToDiscard);
-                    archiveStream.ReadExactly(buffer.AsSpan(0, currentLengthToRead));
-                    bytesToDiscard -= currentLengthToRead;
-                }
-                ArrayPool<byte>.Shared.Return(buffer);
-            }
+            ValueTask vt = AdvanceStreamCoreAsync<SyncReadWriteAdapter>(archiveStream, bytesToDiscard, CancellationToken.None);
+            Debug.Assert(vt.IsCompleted, "Synchronous AdvanceStream completed asynchronously.");
+            vt.GetAwaiter().GetResult();
        }

        // Asynchronously helps advance the stream a total number of bytes larger than int.MaxValue.
-        internal static async ValueTask AdvanceStreamAsync(Stream archiveStream, long bytesToDiscard, CancellationToken cancellationToken)
+        internal static ValueTask AdvanceStreamAsync(Stream archiveStream, long bytesToDiscard, CancellationToken cancellationToken)
        {
-            cancellationToken.ThrowIfCancellationRequested();
+            if (cancellationToken.IsCancellationRequested)
+            {
+                return ValueTask.FromCanceled(cancellationToken);
+            }
+
+            return AdvanceStreamCoreAsync<AsyncReadWriteAdapter>(archiveStream, bytesToDiscard, cancellationToken);
+        }



do we need those two as separate helpers?

I haven't touched gnuspsarsestream, which is where these two are used. the biggest code chunk in there, parsesparsemap, uses the isasync bool. Should I refactor it too?

alinpahontu2912 added 3 commits June 10, 2026 13:27

add readwriteradapter for tarhelpers

0333b56

refactor more reader methods

f6bef1f

refactor write helpers

c0cf61c

alinpahontu2912 requested review from a team and Copilot June 11, 2026 08:56

Copilot started reviewing on behalf of alinpahontu2912 June 11, 2026 08:56 View session

github-actions Bot added the area-System.Formats.Tar label Jun 11, 2026

dotnet-policy-service Bot assigned alinpahontu2912 Jun 11, 2026

Copilot AI reviewed Jun 11, 2026

View reviewed changes

am11 reviewed Jun 11, 2026

View reviewed changes

Comment thread src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReadWriteAdapter.cs

build-analysis Bot mentioned this pull request Jun 11, 2026

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

teo-tsirpanis reviewed Jun 11, 2026

View reviewed changes

rzikm reviewed Jun 11, 2026

View reviewed changes

inline helpers

296c608

Conversation

alinpahontu2912 commented Jun 11, 2026

Uh oh!

dotnet-policy-service Bot commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teo-tsirpanis Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

alinpahontu2912 Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

alinpahontu2912 Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

rzikm Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

alinpahontu2912 Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzikm Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alinpahontu2912 Jun 12, 2026 •

edited

Loading