Refactor sync/async implementations by using Adapter pattern#129282
Refactor sync/async implementations by using Adapter pattern#129282alinpahontu2912 wants to merge 12 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar |
There was a problem hiding this comment.
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
IReadWriteAdapterwithSyncReadWriteAdapter/AsyncReadWriteAdapterimplementations to unify stream operations. - Reworks
TarReader/TarWriterto 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. |
| 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(); |
There was a problem hiding this comment.
Add awaits to help runtime async? Or are they not necessary?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs:640
- WriteEmptyPaddingCoreAsync now rents from ArrayPool even when the padding is at most one tar record (<=512 bytes). The previous synchronous implementation used stackalloc for this small, fixed upper bound, so this introduces per-entry pool traffic on the sync write path.
If the goal is to keep the adapter-based dedup, consider adding a sync-only fast-path for small padding writes (stackalloc + Stream.Write(ReadOnlySpan)) when using SyncReadWriteAdapter, so synchronous archive creation doesn’t pay the ArrayPool rent/return cost per entry.
// Calculates the padding for the current entry and writes it after the data.
private async ValueTask WriteEmptyPaddingCoreAsync<TAdapter>(Stream archiveStream, CancellationToken cancellationToken)
where TAdapter : IReadWriteAdapter
{
int paddingAfterData = TarHelpers.CalculatePadding(_size);
if (paddingAfterData != 0)
{
Debug.Assert(paddingAfterData <= TarHelpers.RecordSize);
byte[] buffer = ArrayPool<byte>.Shared.Rent(paddingAfterData);
try
{
Array.Clear(buffer, 0, paddingAfterData);
await TAdapter.WriteAsync(archiveStream, buffer.AsMemory(0, paddingAfterData), cancellationToken).ConfigureAwait(false);
}
finally
{
ArrayPool<byte>.Shared.Return(buffer);
}
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fixes #127378