Potential ValueTask-related issues in Kestrel

[stephentoub]

I haven't fully investigated all of these, just enough to see that there might be an issue to be followed up on...

1. Ignored results:

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs#L55

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs#L88

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L144

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L1053

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L1063

https://github.com/aspnet/AspNetCore/blob/3ecdc403189a28e2f36723663f50e58c4637e3aa/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs#L103

https://github.com/aspnet/AspNetCore/blob/3ecdc403189a28e2f36723663f50e58c4637e3aa/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs#L470

These are calling a ValueTask-returning method and ignoring the result. If the ValueTask is backed by something that's pooled, not awaiting it could result in that pooled object getting thrown away.

2. Semi-ignored result, and potential reuse:

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs#L67

This is storing a ValueTask into a field, which is then returned from every call to WriteStreamSuffixAsync:

https://github.com/aspnet/AspNetCore/blob/6a6deb298c8b3385213174dd023a45f6a309da5f/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs#L207-L221

Same for HTTP/1.1:

https://github.com/aspnet/AspNetCore/blob/caa910ceeba5f2b2c02c47a23ead0ca31caea6f0/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs#L122

That in turn is used in WriteSuffix:

https://github.com/aspnet/AspNetCore/blob/3ecdc403189a28e2f36723663f50e58c4637e3aa/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L1053-L1058

Two potential issues with that: a) if it's already completed successfully we never call GetAwaiter().GetResult() on it which means if it's pooled it won't be returned, and b) if it wasn't completed and we await it, then if it's used again we're going to be reusing an already consumed instance.

3. Attempting to synchronously block on a ValueTask

https://github.com/aspnet/AspNetCore/blob/caa910ceeba5f2b2c02c47a23ead0ca31caea6f0/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L927

a) Synchronous blocking in ASP.NET?
b) Such use isn't supported on ValueTasks. It may happen to work today based on how the ValueTask was created, but it's in no way guaranteed to.

https://github.com/aspnet/AspNetCore/blob/caa910ceeba5f2b2c02c47a23ead0ca31caea6f0/src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStream.cs#L69-L74

This one is similar. The comment talks about using Result being ok because it calls GetAwaiter().GetResult(), but GetAwaiter().GetResult() isn't guaranteed to block. It'll work ok if the implementation is backed by a value or a Task, but if it's backed by an IValueTaskSource, all bets are off.

cc: @davidfowl

[GrabYourPitchforks]

The "ignored result" presumably should be easy enough to catch via an analyzer. Would an analyzer have caught the other potential misuses? Should we see this as a forcing function for us to write such an analyzer?

[stephentoub]

These are all from an analyzer 😉
dotnet/roslyn-analyzers#3001

[davidfowl]

a) Synchronous blocking in ASP.NET?

HAH, yes, we never fixed the expect 100 continue code path.