'Forget' extensions on ValueTask need to fake an await

[sharwell]

Bug description

ValueTask instances can be backed by pooled objects. If these tasks are not awaited, the pooled objects can get discarded, negatively impacting application efficiency.

Repro steps

Expected behavior

The Forget extension methods behave as though the task is awaited. @stephentoub may be able to help identify the most efficient approach here.

Actual behavior

The task is not awaited.

vs-threading/src/Microsoft.VisualStudio.Threading/TplExtensions.cs

Lines 261 to 278 in 8085d15

	/// <summary>
	/// Consumes a task and doesn't do anything with it. Useful for fire-and-forget calls to async methods within async methods.
	/// </summary>
	/// <param name="task">The task whose result is to be ignored.</param>
	[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "task")]
	public static void Forget(this ValueTask task)
	{
	}
	/// <summary>
	/// Consumes a task and doesn't do anything with it. Useful for fire-and-forget calls to async methods within async methods.
	/// </summary>
	/// <typeparam name="T">The type of value produced by the <paramref name="task"/>.</typeparam>
	/// <param name="task">The task whose result is to be ignored.</param>
	[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "task")]
	public static void Forget<T>(this ValueTask<T> task)
	{
	}

[stephentoub]

public static void Forget(this ValueTask task) => task.AsTask();

would be the simplest. Or:

public static void Forget(this ValueTask task) => task.Preserve();

is probably a tad cheaper, in particular for the ValueTask<T> case.

[AArnott]

Thanks for raising this.
That's disappointing that we have to allocate in order to recycle. Doesn't that seem ironic? And is it even worth it?

[stephentoub]

That's disappointing that we have to allocate in order to recycle.

Preserve won't allocate unless the ValueTask is backed by an IValueTaskSource.

[AArnott]

That's good. It just seems like there ought to be a way to say "you know what, IValueTaskSource? I don't need you any more. Go be recycled now instead of later."

[AArnott]

Disclaimer: ValueTask's various recycling forms aren't something I'm deeply familiar with.

Preserve won't allocate unless the ValueTask is backed by an IValueTaskSource.

But there's nothing to recycle unless there's an IValueTaskSource as well, right? So it sounds like we are allocating exactly if we are recycling. And if there is an IValueTaskSource but the owner doesn't recycle, it's a wasted allocation.

Is this a net win?

[AArnott]

Finally, assuming Preserve is the right thing to call, I wonder how I can write up a test to verify that these Forget methods enable recycling. Can I mock something up so that I can detect when/if Preserve() is called and fail the test if they aren't?

[stephentoub]

It just seems like there ought to be a way to say "you know what, IValueTaskSource? I don't need you any more. Go be recycled now instead of later."

It can't be recycled "now" because it could still be in flight. We could have built in a way to mark it as "when you complete, consider yourself consumed", but you can hook up the continuation yourself. It's just that unless you're in an async method where you already implicitly have the continuation object already allocated, you would need to allocate something for the callback to know which ValueTask instance to access.

So it sounds like we are allocating exactly if we are recycling.

Yes, but please share with me in what situation you're getting back a ValueTask that might be backed by an IValueTaskSource and where you want to "forget" it. Sounds highly dubious. APIs should really only return ValueTask if the 99.999% usage pattern is for them to be awaited. If you're hitting the allocation case in any frequency that would cause perf issues, you have a deeper problem.

ValueTask's various recycling forms aren't something I'm deeply familiar with.

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

Is this a net win?

Even needing a method like "Forget" for ValueTask is an anti-pattern; it should basically never be needed. And in the rare circumstance where you do call it with a method that returned a ValueTask backed by a value or a Task, there's no allocation. It's only if you're calling it with a ValueTask backed by an IValueTaskSource that you're paying for an allocation, the same allocation you would have had anyway if the method returned Task.

Finally, assuming Preserve is the right thing to call, I wonder how I can write up a test to verify that these Forget methods enable recycling.

Write your own IValueTaskSource implementation, create a ValueTask around it, call Forget, and validate that your GetResult method is called once and only once.

[AArnott]

Thanks.

Even needing a method like "Forget" for ValueTask is an anti-pattern; it should basically never be needed.

Would you say the same for our pre-existing Task.Forget() extension method, or do you feel it's worse for ValueTask? We use it on Task to avoid compiler or analyzer warnings when we invoke async methods but don't care to await them or do anything else with them. We generally avoid doing that unless we know the async method handles all its own exceptions, and even then, I advocate using it sparingly. But it is more obvious and searchable to use .Forget() than to assign to a local discard variable, so IMO there is value to these Forget() methods.

I added the extension methods for ValueTask too when I recently came across the same need to discard the result that I've had for Task in other cases.

[stephentoub]

do you feel it's worse for ValueTask?

Yes.

I realize my perspective here may differ from folks who believe every single possible allocation ever should be avoided at all costs. Anyone who knows me in a professional capacity knows I care deeply about performance, but even so I think it needs to be more measured than this. My perspective is that Task should continue to be the default choice as the return type from asynchronous methods, and that ValueTask should only be used as the result of methods where:

• they're intended to always be awaited, and
• they're likely to be used on hot paths

(Stream is a good example. ReadAsync benefits significantly from using ValueTask<int>, as it avoids an allocation per call on what's often a hot path, and it's exceedingly rare to want to read but then ignore the result.)

Task has fewer pitfalls than ValueTask. If you only ever await the result of an async method, then the pitfalls of ValueTask largely go away, but the problem is that consumers can easily miss the differences, and do things like await ValueTasks twice (or worse, concurrently), try to use .GetAwaiter().GetResult() to block waiting for them to complete, etc. I'm finishing up dotnet/roslyn-analyzers#3001 to help with this, but as we all know analyzers can only go so far.

If one adheres to the above guidance, then there should be many, many fewer cases where you'd care to use something like Forget with ValueTask, and the need to use Forget would suggest a mistake on the part of the API developer in returning ValueTask. There are a few more cases internal to a library where it might choose to have some private method returning a ValueTask and then ignore it, but that's self-contained and the developer both authoring and consuming that same method needs to understand the implications of what they're doing.

I realize there's some circular logic in the above, but it's how I think about it currently.

when I recently came across the same need to discard the result that I've had for Task in other cases.

Can you share some details on in what situation this occurred?

[AArnott]

Thanks for the guidance.

These extension methods are only 10 days old and haven't been inserted to VS yet AFAIK, so removing them entirely is absolutely a possibility. I'm content to remove them as it would seem to help foster these conversations with others who as you say may be over-using ValueTask.

Can you share some details on in what situation this occurred?

I'm afraid I don't remember which repo I was working in at the time when I wished I had this.

[AArnott]

I was just about to add a commit to my PR to actually remove the methods, but I had another thought that makes me hesitate:

Since I learned that "forgetting" a ValueTask isn't as simple as it was with Task, it's not so trivial for someone else to workaround the lack of a Forget() extension method. I thought the value of these Forget() methods would be quite low (simply a substitute for assigning the result to a discard variable). But it turns out that doing the obvious thing is wrong (leading to this bug in the first place), so I wonder if we should keep these methods here so that folks who are used to calling our Task.Forget() extension method will at least be avoiding the same mistake I had made.

What is your recommendation, @stephentoub? I'll go either way based on your suggestion.

[stephentoub]

I can see an argument either way. It's a bit of the tail-wagging-the-dog: my concern with Forget is really that it's blessing the producer's choice to return ValueTask in a situation where a consumer is likely to need something like Forget. Once the API creator has chosen to do so, it's not the consumers fault if they end up needing fire-and-forget, and we should help them do it correctly. So, I think it's ok to keep them, but consider adding an XML comment warning that makes it clear it's not ideal.