odata.net icon indicating copy to clipboard operation
odata.net copied to clipboard

Dispose calls in constructors can lead to unwanted synchronous I/O.

Open habbes opened this issue 1 year ago • 1 comments

We have cases in ODL code where we call Stream.Dispose() in the constructor, in a pattern like this:

public ODataJsonLightOutputContext(...)
{
   try
   {
       this.messageOutputStream = messageInfo.MessageStream;
       // ...
    }
    catch (Exception e)
    {
        if (ExceptionUtils.IsCatchableExceptionType(e))
        {
           this.messageOutputStream.Dispose()
         }
     }
}

This pattern exists in: ODataJsonLightOutputContext, ODataMetadataJsonOutputContext, ODataMetadataOutputContext, ODataMetadataRawOutputContext and probably other areas.

Since these classes are also instantiated in async code, it may lead to unwanted synchronous I/O in async code, which may lead to exceptions in production, thread starvation or other issues. We should refactor these call sites to get the Dispose() calls out of the constructor and use DiposeAsync() in async code. I don't know of any actual cases where this has blown up in production, but we have had other cases of synchronous I/O in async code causing issues in production.

Assemblies affected

Microsoft.OData.Core 7.x

habbes avatar Jun 12 '23 09:06 habbes