Implement Tar async APIs

[carlossanlop]

Addresses: #65951

Submitting it as a draft for these reasons:

• This PR depends on another that needs to be merged first: Add TarEntry conversion constructors #70325
• I want to first show each individual async method implemented identically to the synchronous one, with the asynchronous differences only.
• Add unit tests.
• Public documentation.
• Remove duplicate code between sync and async methods.
• Address feedback.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Addresses: #65951

Submitting it as a draft for these reasons:

• This PR depends on another that needs to be merged first: Add TarEntry conversion constructors #70325
• I show each individual async method implemented identically to the synchronous one, with the asynchronous differences only.
• I collect some feedback in case any of those calls needs improvement. The first commit with async code is 2e5304330aede14c8bd7ef2f852f90a8a59aade9.
• I remove duplicate code between sync and async methods.
• Add unit tests.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	area-System.IO
Milestone:	7.0.0

[stephentoub]

Typically any exceptions other than argument exceptions are wrapped in the returned task, e.g.

return Task.FromException(new InvalidOperationException(string.Format(SR.TarEntryTypeNotSupportedForExtracting, EntryType));

[carlossanlop]

Will do.

Why do we not wrap argument exceptions? And what about ArgumentNullException?

[carlossanlop]

This feels like a good candidate for a Roslyn analyzer, if we don't have one yet.

[stephentoub]

Why do we not wrap argument exceptions?

Because in typical usage they represent a 100% deterministic developer error in how the API is being used. We don't want such errors being eaten by the returned task, especially if doing so could lead to a deadlock, e.g. if this task won't end up performing some work that would ultimately unblock something else.

And what about ArgumentNullException?

That is an argument exception. So same deal.

[carlossanlop]

Thanks for the explanation.

I opened this API suggestion in case it makes sense to add such analyzer: #70905

[carlossanlop]

I know there isn't one, but my question was around handling the exception.

[stephentoub]

There's no reason to create an asynchronous version of ExtractAsHardLink or CheckIo, so I'm not clear on your question. Can you clarify?

[carlossanlop]

So the async method that calls ExtractAsHardLink should wrap that in a try catch, and the catch should return Task.FromException?

internal Task MethodAsync(...)
{
try
{
    ExtractAsHardLink(...);
    return Task.CompletedTask;
}
catch (Exception e)
{
    return Task.FromException(e);
}
}

[carlossanlop]

In other words, I should only create a new async method if it will call another async method inside it.

But in the case of methods that can exceptions, the last async method in the callstack is the one which should handle the exception by catching it and return it inside a Task.

Is this correct?

[bartonjs]

What really matters is the public function black-box experience.

In the call of

Task t = someObjectOrType.FooAsync(arg0, arg1, arg2);
t.Wait();
Console.WriteLine(t.Exception);

There are two possibilities for the exception:

1. the call to FooAsync was invalid, in which case the FooAsync method itself should throw, and we never make it to line 2. (ArgumentException)
2. We created a Task. The task ran... until it failed. The exception is now in the t.Exception property.

For the latter, this is accomplished in one of three ways.

I) After argument validation, FooAsync failed synchronously, and the implementation returns Task.FromException(e).
II) You're building tasks by chaining them together with Task.ContinueWith(action), inside the action you throw an exception (as per normal). The Task system records the exception and changes the task state to Faulted.

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Lines 2332 to 2357 in 988f570

	try
	{
	ExecutionContext? ec = CapturedContext;
	if (ec == null)
	{
	// No context, just run the task directly.
	InnerInvoke();
	}
	else
	{
	// Invoke it under the captured ExecutionContext
	if (threadPoolThread is null)
	{
	ExecutionContext.RunInternal(ec, s_ecCallback, this);
	}
	else
	{
	ExecutionContext.RunFromThreadPoolDispatchLoop(threadPoolThread, ec, s_ecCallback, this);
	}
	}
	}
	catch (Exception exn)
	{
	// Record this exception in the task's exception list
	HandleException(exn);
	}

III) You're writing a method with async/await. That's just a fancy way of doing II.

What that boils down to is:

• In the public method you might need to be aware of being "inside" or "outside" the task with an exception, here's where Task.FromException comes in.
• Once you enter an async/await method you can stop thinking, just write throw as normal.
• If you're making Tasks by hand, and you're inside a callback, throw as normal.
• If you're making Tasks by hand and you're not inside a callback, Task.FromException may again matter.
  • But why are you making Tasks by hand after the public entrypoint?

[stephentoub]

This should use ArrayPool directly rather than MemoryPool.

[carlossanlop]

But a couple of lines below I need to pass the Memory<byte> to archiveStream.ReadExactlyAsync.

[bartonjs]

Sure... but arrays are implicitly Memory<byte> (or if you want to slice it, then explicitly use AsMemory)

byte[] rented = ArrayPool<byte>.Shared.Rent(TarHelpers.RecordSize);
Memory<byte> buffer = rented.AsMemory(0, TarHelpers.RecordSize);

...

ArrayPool<byte>.Shared.Return(rented);

[carlossanlop]

TIL. Thanks.

[carlossanlop]

Sure... but arrays are implicitly Memory<byte> (or if you want to slice it, then explicitly use AsMemory)

Thanks for this explanation in advance. I am starting to see some test failures when attempting to do async writes using the existing code that takes spans, like this:

runtime/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

Lines 207 to 212 in 70e9ca0

	private int WriteName(Span<byte> buffer, out byte[] fullNameBytes)
	{
	fullNameBytes = Encoding.ASCII.GetBytes(_name);
	int nameBytesLength = Math.Min(fullNameBytes.Length, FieldLengths.Name);
	int checksum = WriteLeftAlignedBytesAndGetChecksum(fullNameBytes.AsSpan(0, nameBytesLength), buffer.Slice(FieldLocations.Name, FieldLengths.Name));
	return checksum;

[carlossanlop]

@stephentoub this public API returns a Task. My immediate assumption was that all the internal async APIs this method calls should also return a Task. Is there anything preventing me from changing all the internal methods to return a ValueTask instead?

[stephentoub]

Is there anything preventing me from changing all the internal methods to return a ValueTask instead?

No, but just making that change will typically only yield benefits in the case of Task<T> to ValueTask<T>.

[bartonjs]

The ArgumentException, and possibly the ObjectDisposedException should generally be "outside" the Task. So this method shouldn't be public async Task, just public Task, do argument validation, then it can call an async Task private or local member.

[bartonjs]

After switching ot ArrayPool, move this to the try, before the return, but not the finally.

Async methods shouldn't return to the pools via a finally

[bartonjs]

(Which, I guess, means you can get rid of the whole try/finally)

[stephentoub]

Async methods shouldn't return to the pools via a finally

Why?

(In general I think it's ok to not return to the array pool in the case of an exception, on the assumption that exceptions are rare, but that goes for sync and async.)

[bartonjs]

I'm mainly parroting Levi here for "don't return to a pool from a finally in async code", but the reasoning had something to do with the possibility of a native async operation still doing writes when the managed wrapper task has thrown a timeout exception, so returning to the pool in that case could result in the async op writing to the buffer after it has been rented elsewhere.

I remember reading a guidance document at some point, but I can't find it offhand.

From synchronous code, the expectation is more that there shouldn't be a possibility of a second reference, so returning from a finally is OK (though my recollection was that the guidance suggested not to, for a uniformity of style).

[stephentoub]

but the reasoning had something to do with the possibility of a native async operation still doing writes when the managed wrapper task has thrown a timeout exception, so returning to the pool in that case could result in the async op writing to the buffer after it has been rented elsewhere.

That would be a bug in such an async method. The task returned from an async method should not complete until all work associated with the operation has quiesced, all resources are no longer used, etc.

From synchronous code, the expectation is more that there shouldn't be a possibility of a second reference, so returning from a finally is OK (though my recollection was that the guidance suggested not to, for a uniformity of style).

The exact same issue could exist with synchronous code, if the implementation similarly erroneously returned/threw while resources were still in use.

[carlossanlop]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[adamsitnik]

I've reviewed 12 out of 64 files, will get back to this PR later today after I have some more coffee.

[adamsitnik]

it seems like there is no need to check for the cancellation again here, as ReadAsync (the caller) always does that

Suggested change

cancellationToken.ThrowIfCancellationRequested();

[carlossanlop]

I was under the impression that the token cancellation status should be checked on every method.

[adamsitnik]

I was under the impression that the token cancellation status should be checked on every method

As long as some work was executed between them (so it could have actually changed in the meantime). At least this is how I see it.

[adamsitnik]

I currently don't have a good suggestion for how this method could be refactored to reduce code duplication. I'll try to play with the code once your PR gets merged.

[carlossanlop]

I know. TarHeader is the problem, since it is a struct. I may have to refactor the code later to get rid of TarHeader completely, move everything to TarEntry.

[adamsitnik]

why the sync method writes the checksum to a field and the async method does not?

[carlossanlop]

TarHeader is a struct. It owns the _checksum field. In the sync method, I have no problem updating the value of _checksum inside the method: the target field gets updated. But in the async method, the TarHeader is copied into the method, so the _checksum value that gets updated is the one from the copied header, not the original one.

As discussed on chat, this is a good candidate for refactoring and simplification on a later PR.

[adamsitnik]

It seems that we could reduce code duplication here by moving part of the common logic to a separate method:

private long WriteCommonFieldsAndChecksumAsV7(Span<byte> buffer)
{
    long actualLength = GetTotalDataBytesToWrite();
    TarEntryType actualEntryType = GetCorrectTypeFlagForFormat(TarEntryFormat.V7);

    int checksum = WriteName(buffer, out _);
    int checksum = WriteNameSpan(buffer, out _);
    checksum += WriteCommonFields(buffer, actualLength, actualEntryType);
    WriteChecksum(checksum, buffer);
    _checksum = WriteChecksum(checksum, buffer);
    
    return actualLength;
}

internal void WriteAsV7(Stream archiveStream, Span<byte> buffer)
{
    long actualLength = WriteCommonFieldsAndChecksumAsV7(buffer);

    archiveStream.Write(buffer);

    if (_dataStream != null)
    {
        WriteData(archiveStream, _dataStream, actualLength);
    }
}

internal async Task<int> WriteAsV7Async(Stream archiveStream, Memory<byte> buffer, CancellationToken cancellationToken)
{
    cancellationToken.ThrowIfCancellationRequested();

    long actualLength = WriteCommonFieldsAndChecksumAsV7(buffer.Span);

    await archiveStream.WriteAsync(buffer, cancellationToken).ConfigureAwait(false);

    if (_dataStream != null)
    {
        await WriteDataAsync(archiveStream, _dataStream, actualLength, cancellationToken).ConfigureAwait(false);
    }

    return _checksum;
}

[adamsitnik]

similar to the method above, can we reduce the code duplication between sync and async versions?

[adamsitnik]

code duplication

[adamsitnik]

code duplication?

[adamsitnik]

code duplication

[adamsitnik]

code duplication?

[carlossanlop]

I'm seeing a weird behavior in some async tests in Linux, and I was able to reproduce it locally under certain conditions.

Take for example the test System.Formats.Tar.Tests.TarFile_ExtractToDirectoryAsync_File_Tests.Extract_Archive_File_OverwriteFalse_Async:

[Fact]
public Task Extract_Archive_File_OverwriteFalse_Async()
{
    string sourceArchiveFileName = GetTarFilePath(CompressionMethod.Uncompressed, TestTarFormat.pax, "file");
    using TempDirectory destination = new TempDirectory();
    string filePath = Path.Join(destination.Path, "file.txt");
    File.Create(filePath).Dispose();
    return Assert.ThrowsAsync<IOException>(() => TarFile.ExtractToDirectoryAsync(sourceArchiveFileName, destination.Path, overwriteFiles: false));
}

I made it non-async, and it is returning the Task directly.

If I execute all the tests locally using dotnet build /t:test System.Formats.Tar.Tests.csproj, all the tests pass.

But if I execute that test individually using dotnet build /t:test System.Formats.Tar.Tests.csproj /p:XUnitMethodName=System.Formats.Tar.Tests.TarFile_ExtractToDirectoryAsync_File_Tests.Extract_Archive_File_OverwriteFalse_Async, then it fails with the following error:

Starting:    System.Formats.Tar.Tests (parallel test collections = on, max threads = 12)
      System.Formats.Tar.Tests.TarFile_ExtractToDirectoryAsync_File_Tests.Extract_Archive_File_OverwriteFalse_Async [FAIL]
        Assert.Throws() Failure
        Expected: typeof(System.IO.IOException)
        Actual:   (No exception was thrown)

If I change the method and make it async, and make it await the Task instead of returning it, then it passes:

[Fact]
public async Task Extract_Archive_File_OverwriteFalse_Async()
{
    ...
    await Assert.ThrowsAsync<IOException>(() => TarFile.ExtractToDirectoryAsync(sourceArchiveFileName, destination.Path, overwriteFiles: false));
}

Anyone knows if this is expected or is it a bug? Regardless of the answer, it seems I'll have to change the tests that return a Task so they instead await it.

[stephentoub]

I made it non-async, and it is returning the Task directly.

You have a race condition. Your code:

    using TempDirectory destination = new TempDirectory();
    string filePath = Path.Join(destination.Path, "file.txt");
    File.Create(filePath).Dispose();
    return Assert.ThrowsAsync<IOException>(() => TarFile.ExtractToDirectoryAsync(sourceArchiveFileName, destination.Path, overwriteFiles: false));

is using a using on the TempDirectory. The moment you return the task from ThrowsAsync, that destination will be disposed. That may or may not be before your ExtractToDirectoryAsync call has completed.

[carlossanlop]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[adamsitnik]

LGTM, great work Carlos!

I found some issues, but they are related to style (nits) and code duplication, not actual bugs. So please fell free to merge the PR now and we can work on the refactor later.

The amount of tests is insane! I'll be dreaming of fact and theories after reading source code of so many tests ;)

[adamsitnik]

the finalizer should be calling the synchronous dispose method, not the async one. And this is how it's implemented, but the comment above (most likely due to copy paste-error) is incorrect. So disposing will be always true for this method. IMO we should remove the parameter and inline this method into the public DisposeAsync

[bartonjs]

Since the type is sealed it makes sense to do all of the DisposeAsync logic in DisposeAsync, instead of DisposeAsyncCore() (which this method should have been called for the basic async dispose pattern).

Similarly, Dispose(bool) can be merged into Dispose(), and all of the GC.SuppressFinalize(this)s can be removed.

[adamsitnik]

code duplication

[adamsitnik]

code duplication

[adamsitnik]

code duplication

[adamsitnik]

ditto

[adamsitnik]

nit: it could be const

Suggested change

	string testCaseName = "file";
	const string testCaseName = "file";

[adamsitnik]

the new C# syntax for using does not support async using? that's a pitty

[carlossanlop]

Yep, it does. I was initially working on VS Code and I was getting errors saying that syntax wasn't supported. So I submitted changes like this one.

But then I started using Visual Studio 2022 again, and tried using the regular using syntax, and it works. So I joined most of the await usings like this one in my last commit, but missed a few others. I'll push another commit merging the rest of the await usings with the declaration of the disposable objects.

[adamsitnik]

I was under the impression that the token cancellation status should be checked on every method

As long as some work was executed between them (so it could have actually changed in the meantime). At least this is how I see it.