OData / odata.net

ODataLib: Open Data Protocol - .NET Libraries and Frameworks
https://docs.microsoft.com/odata
Other
687 stars 349 forks source link

Provide async API for CsdlWriter #2684

Open habbes opened 1 year ago

habbes commented 1 year ago

The CsdlWriter only exposes synchronous APIs for writing CSDL (WriteCsdl and TryWriteCsdl). This means that any async API that relies on CsdlWriter to serialize CSDLs (e.g. ODataMessageWriter.WriteMetadataAsync) will have to perform synchronous I/O, which may lead to undesired effects (sync over async which may lead to thread starvation, exceptions in environments that don't allow synchronous I/O, etc.).

Assemblies affected

Micrososft.OData.Edm 7.x

Reproduce steps

The simplest set of steps to reproduce the issue. If possible, reference a commit that demonstrates the issue.

Expected result

Actual result

Additional detail

You may be wondering: If ODataMessageWriter.WriteMetadataAsync performs synchronous I/O and ASP.NET Core disables synchronous I/O by default then how come /$metadata request in ASP.NET Core OData do not throw exceptions? Well, ASP.NET Core OData 8 implements a hack to get around this issue. It has a StreamWrapper that implements synchronous I/O calls like Flush() using blocking async calls calls like FlushAsync().Wait() (which is also bad for perf). We should also address this when we make WriteMetadataAsync truly async.

For customers who use ODataMessageWriter directly and don't want to perform synchronous I/O, a workaround could be to write the CSDL in a memory stream and cache the byte array, then use the byte array as the input for async I/O.

joaocpaiva commented 1 year ago

Well, ASP.NET Core OData 8 implements a hack to get around this issue. It has a [StreamWrapper](https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Formatter/ODataOutputFormatterHelper.cs#L244) that implements synchronous I/O calls like Flush() using blocking async calls calls like FlushAsync().Wait() (which is also bad for perf). We should also address this when we make WriteMetadataAsync truly async.

Using Task.Wait(), Task.GetAwaiter().GetResult() or Task.Result also known as sync over async anti-pattern, is even worse than calling Flush() because when we do sync over async we are holding 2 threadpool threads hostages to perform an operation that should be done in 1 thread (worst case) to complete the operation which can induce threadpool starvation.

habbes commented 2 months ago

Fixed for ODL 8, but not ODL 7.