Fix copy cancelation

[danmoseley]

Fix #3891

Ctrl-C calls Cancel() on the task. That signals the cancelation token we pass into the ActionBlock which throws an exception that we do not catch.

Note: the exception is caught in two other places; those are still required as they would otherwise be caught in the generic catch there and log and error.

No unit test here is really feasible without exceptional efforts.

[JanKrivanek]

Looks good.

Do we maybe want to remove the IsCancellationRequested check https://github.com/dotnet/msbuild/pull/8951/files#diff-bfdedf8860a0dc992c6729c5119fb3ae2d56fc4587d803160434089181cbb421R441?

[rainersigwald]

Looks like we need a task-canceled message:

C:\play>dotnet msbuild foo.proj
MSBuild version 17.7.0-preview-23281-03+4ce2ff1f8 for .NET
  foo Go (0.6s)
  foo failed with errors (1.5s)
    ❌ C:\play\foo.proj(7,9): error MSB4018: The "Copy" task failed unexpectedly.
C:\play\foo.proj(7,9): error MSB4018: System.Threading.Tasks.TaskCanceledException: A task was canceled.
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.CopyParallel(CopyFileWithState copyFile, Int32 parallelism, List`1& destinationFilesSuccessfullyCopied)
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.Execute(CopyFileWithState copyFile, Int32 parallelism)
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.Execute()
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask)

Build failed with errors in 1.6s

C:\play>S:\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe
MSBuild version 17.8.0-dev-23326-01+92f360082 for .NET Framework
  foo Go (0.5s)

Build failed in 1.9s

Do we maybe want to remove the IsCancellationRequested check https://github.com/dotnet/msbuild/pull/8951/files#diff-bfdedf8860a0dc992c6729c5119fb3ae2d56fc4587d803160434089181cbb421R441?

It's probably not important either way. It'd be nice to fail if cancelled even if cancelled right after finishing everything but bookkeeping--but doesn't matter much.

[danmoseley]

Looks like we need a task-canceled message:

Was that with these changes? TaskCanceledException is-a OperationCanceledException, so this change should take care of both.

It's probably not important either way. It'd be nice to fail if cancelled even if cancelled right after finishing everything but bookkeeping--but doesn't matter much.

If cancel happens just after the catch block, there is no way for the task to not succeed -- because there's no code in between that respects the cancelation token. Ideally MSBuild itself marks tasks as failed if it signals the token before the task returns - if it doesn't, it probably should.

So as you say I can do it either way. I suggest to leave as is as it's "correct" and requires no reasoning.

[JanKrivanek]

Looks like we need a task-canceled message:

C:\play>dotnet msbuild foo.proj
MSBuild version 17.7.0-preview-23281-03+4ce2ff1f8 for .NET
  foo Go (0.6s)
  foo failed with errors (1.5s)
    ❌ C:\play\foo.proj(7,9): error MSB4018: The "Copy" task failed unexpectedly.
C:\play\foo.proj(7,9): error MSB4018: System.Threading.Tasks.TaskCanceledException: A task was canceled.
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.CopyParallel(CopyFileWithState copyFile, Int32 parallelism, List`1& destinationFilesSuccessfullyCopied)
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.Execute(CopyFileWithState copyFile, Int32 parallelism)
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.Tasks.Copy.Execute()
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
C:\play\foo.proj(7,9): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask)

Build failed with errors in 1.6s

C:\play>S:\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe
MSBuild version 17.8.0-dev-23326-01+92f360082 for .NET Framework
  foo Go (0.5s)

Build failed in 1.9s

Do we maybe want to remove the IsCancellationRequested check https://github.com/dotnet/msbuild/pull/8951/files#diff-bfdedf8860a0dc992c6729c5119fb3ae2d56fc4587d803160434089181cbb421R441?

It's probably not important either way. It'd be nice to fail if cancelled even if cancelled right after finishing everything but bookkeeping--but doesn't matter much.

This is weird - the exception should have been swallowen. Do you want to retry @rainersigwald ?
Keeping unmerged for one more day if anybody want's to raise any red flag (like this one) - then I'll merge

[danmoseley]

@rainersigwald was your stack above with this change in place?

[rainersigwald]

No, the stack was without this change, the latter (failed with no error) was with it. I think #8975 is a good way to fix that.

I think we should take this change.

[danmoseley]

Ah right. Hmm, when does "attempting to cancel the build" appear I wonder?

But yes let's merge this

[rainersigwald]

when does "attempting to cancel the build" appear I wonder?

I think it'd work better if I hadn't opted into the new logger: #8983.