Cancellation2 #5288
Cancellation2 #5288
Conversation
samples/Playground/Tests.cs
Outdated
| @@ -15,7 +15,7 @@ public class TestClass | |||
| [TestMethod] | |||
| [DynamicData(nameof(Data))] | |||
| public void Test3(int a, int b) | |||
| => throw new Exception("aaaa"); | |||
| => Thread.Sleep(30_000); | |||
There was a problem hiding this comment.
Revert before merging please (ideally keep the test method empty)
There was a problem hiding this comment.
yes sure, it is a draft to see if I pass tests. It passes abortion tests locally.
| await ExecuteRequestAsync(testFramework, request, messageBus, cancellationToken); | ||
| var taskCompletionSource = new TaskCompletionSource(); | ||
| using CancellationTokenRegistration r = cancellationToken.Register(() => taskCompletionSource.TrySetCanceled()); | ||
| await Task.WhenAny(ExecuteRequestAsync(testFramework, request, messageBus, cancellationToken), taskCompletionSource.Task); |
There was a problem hiding this comment.
Here we ensure that the request run can be abandoned without completing when canelation token is cancelled (this token is test session cancellation token).
This will give the test framework a chance to run CloseTestSessionAsync https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/TestFramework/ITestFramework.cs
which seems to align with the intent of the interface and the token.
There was a problem hiding this comment.
That makes it pointless to pass a cancellation token to framework authors right? Because now we will force-cancel on our side.
I'd expect framework authors to handle it. So, if we want to change something, I'd change in MSTest rather than MTP
There was a problem hiding this comment.
And thinking more, this is too risky. If a cancellation is requested, we may be proceeding to CloseTestSessionAsync while ExecuteRequestAsync is still running right? We just bypassed it by the WhenAny but it's still there and running
There was a problem hiding this comment.
That makes it pointless to pass a cancellation token to framework authors right?
I don't think it is pointless. We pass the same token within the whole test session via Services.TestSessionContext. This cancellation token is called abortRun.
The test framework has 3 callbacks, before test session, execute request, and after test session.
In TestHostFrameworkInvoker we pass the abortTestRun to all those 3 callbacks. And before each operation we check if we should do more work, or if cancellation was requested.
If it was requested, we abort, if not we continue.
Problem is when long running work starts in the testFramework.ExecuteRequestAsync, we don't have a way out, which is what this PR implements.
When executeRequestAsync is running long synchronous work, we cancel the token, we will ignore that running work and go directly to test session teardown that is provided by the framework.
Once the single item of work finishes in testframework.executerequestasync, no more work should be started because framework author should check for cancellation before starting any work. This is very similar to what we do internally in mstest when timeout for test is specified and the test times out (IMHO).
The risky point is what this PR is trying to achieve, so we don't have to rely on framework authors to not block the platform from completing. This is non-graceful teardown of the framework.
To handle this predictably, there needs to be a whole other layer (or two).
The testframework.ExecuteRequest should get a separate cancelation token that will cancel the run, and there should be a different gesture for Cancel of test run (which is more graceful) and Abort (which is less graceful but still not as aggressive as killing the process), which is what we now call Cancel.
| @@ -68,6 +70,8 @@ public async Task ExecuteAsync(ITestFramework testFramework, ClientInfo client, | |||
|
|
|||
| public virtual async Task ExecuteRequestAsync(ITestFramework testFramework, TestExecutionRequest request, IMessageBus messageBus, CancellationToken cancellationToken) | |||
| { | |||
| await Task.Yield(); | |||
There was a problem hiding this comment.
here we ensure that this method will run asynchronously, so we can handle the Task.WhenAny above in that call stack.
| @@ -68,6 +70,8 @@ public async Task ExecuteAsync(ITestFramework testFramework, ClientInfo client, | |||
|
|
|||
| public virtual async Task ExecuteRequestAsync(ITestFramework testFramework, TestExecutionRequest request, IMessageBus messageBus, CancellationToken cancellationToken) | |||
| { | |||
| await Task.Yield(); | |||
|
|
|||
| using SemaphoreSlim requestSemaphore = new(0, 1); | |||
| await testFramework.ExecuteRequestAsync(new(request, messageBus, new SemaphoreSlimRequestCompleteNotifier(requestSemaphore), cancellationToken)); | |||
There was a problem hiding this comment.
Not sure why the semaphore slim is needed here, instead of just completing and awaiting the task. The semaphore is passed in via context and test frameworks are supposed to call Complete on it.
There was a problem hiding this comment.
https://github.com/thomhurst/TUnit/blob/main/TUnit.Engine/Framework/TUnitTestFramework.cs#L170 this code does not do it, but in theory the ExecuteRequestAsync can complete and pass the context to something else, which will then call Complete when done.
Seems rather equivalent to just awaiting the task. Because we will still await some completion.
There was a problem hiding this comment.
Unless there is something I don't see, I think it could be reasonable to consider changing that, but probably in v2 as Complete is public already. So we can remove Complete and rely on the returned task. If framework authors want to "complete" on another thread or whatever, they could create TCS and set result in whatever place they would normally call "Complete" in. That's separate from cancellation though
Possibly better implementation for (mostly) non-cooperative cancellation that is not dependent on framework.