Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add CallTarget support for
ValueTask
in .NET FX and < .NET Core 3.1 (…
…#6480) ## Summary of changes Add support for correct `CallTarget` instrumentation of methods that return `ValueTask` in < .NET Core 3.1 or .NET Framework ## Reason for change We already support instrumenting methods that returns `ValueTask` in .NET Core 3.1. However, in .NET Framework or .NET Standard 2.0, this support is [provided by a package](https://www.nuget.org/packages/System.Threading.Tasks.Extensions), and we currently _don't_ support instrumenting these methods. Or rather, we just ignore the `OnAsyncMethodEnd` in integrations in these cases. ## Implementation details We already support `ValueTask` in more recent frameworks, and the support is very similar to our `Task` support. Unfortunately, in .NET FX we can't reference the `ValueTask` type itself. To work around this, we do the following: - Detect that the return type is `ValueTask` or `ValueTask<T>` either by loading the type directly (.NET Core) or checking the type name (.NET Framework) - Duck-type the `ValueTask` to read the `IsCompletedSuccessfully` value. - If this is true, the task is already completed synchronously, and we can simply call the target method. - For `ValueTask<T>` we duck type `Result` and read that directly too. - If it _hasn't_ completed, we need to extract the `Task` from it. - Duck typing is used again to extract the `Task()` for uncompleted `ValueTask` - At this point, it's mostly a copy-paste of the existing `Task` integrations - Once the integration returns, we need to create a "new" `ValueTask` instead from the previous one - The semantics of `ValueTask` _require_ that we create a "fresh" one, we can't just "reuse" the one we got originally, because we've already retrieved the result/ awaited the inner task - Have to use `Activator`/`DynamicMethod` for this > [!WARNING] > The existing `ValueTask`/`ValueTask<>` and `Task`/`Task<>` integrations are written quite differently, and I'm not entirely sure why 🤔 Given the `ContinuationAction()` methods for both these cases operate on `Task`, I based on the new `ValueTask` integrations on the `Task` integrations, but if anyone has reasons why it shouldn't be, I'm all ears! ## Test coverage - Added `Task` and `ValueTask` tests to the `CallTargetNativeTests` integration tests. Previously we were only testing a single `Task` example, and that was somewhat insufficient - Update the `CallTargetNativeTests` to explicitly assert that the `OnAsyncMethodEnd` methods are called for `Task` / `ValueTask` method integrations. As we provide _both_ methods in our target integration, we were silently calling the wrong one for .NET FX - Prior to the fix in this PR, these updated tests would fail on < .NET Core 3.0 and .NET FX - Run the `ValueTaskAsyncContinutationGenerator` unit tests on all TFMs, not just .NET Core 3.1 - Unit tests for the `ValueTaskHelper` for checking if a type is a `ValueTask` - Unit tests for the `ValueTaskActivator` for creating a `ValueTask` from a `Task` or `Task<T>` - Verified it fixes the issues I was seeing in the RabbitMQ integration - #6479 ## Other details Required for - #6479
- Loading branch information