Being sometimes required to call a particular method before an object is disposed emits a pungent code smell. In particular recently reviewed code, invoking an asynchronous Flush() method was sometimes necessary before a MyStream object was disposed, because asynchronous methods would be otherwise called from Dispose(), which does not wait for completion of those asynchronous methods.
An exception encountered with the following code sample motivates the workaround described above:
using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
class MyService : IDisposable
{
private MemoryStream ms;
public MyService(MyStream myStream)
{
ms = myStream.GetStream();
}
public async Task WriteAsync(string s)
{
byte[] buf = System.Text.Encoding.ASCII.GetBytes(s);
await ms.WriteAsync(buf, 0, s.Length);
}
public async Task Flush()
{
try
{
await Task.Delay(50);
int length = (int)ms.Length;
byte[] buf = new byte[length];
ms.Seek(0, System.IO.SeekOrigin.Begin);
await ms.ReadAsync(buf, 0, length);
foreach (byte b in buf)
Console.Write((char)b);
Console.WriteLine("");
}
catch (Exception ex)
{
Console.WriteLine(ex.ToString());
}
Program.Done();
}
public async void Dispose()
{
await Flush();
}
}
class MyStream : IDisposable
{
MemoryStream ms = new MemoryStream();
public MemoryStream GetStream()
{
return ms;
}
public void Dispose()
{
ms.Close();
}
}
class Program
{
private static EventWaitHandle ewh =
new EventWaitHandle(false, EventResetMode.ManualReset);
public static void Done()
{
ewh.Set();
}
public static async void Run()
{
using (var myStream = new MyStream())
{
using (var myService = new MyService(myStream))
{
await myService.WriteAsync("success");
}
}
}
public static void Main(string[] args)
{
Run();
ewh.WaitOne();
}
}
Calling async methods from Dispose(), which is non-async or async void, potentially causes the following exception:
System.ObjectDisposedException: Cannot access a closed
Stream.
at System.IO.MemoryStream.get_Length()
at MyService.Flush() in ...\Program.cs:line 22
The exception is caused by Flush() accessing the Length property of a MemoryStream object that has already been disposed. Although the lifetime of myStream is expected to encompass that of myService, the Flush() method is not actually performed until after the myStream object is invalidated by MyStream.Dispose().
MyService.Dispose() does not wait for Flush() to complete even though “async” is added to the MyService.Dispose() method signature and await is used on the Flush() call.1
Also, async void is “generally discouraged for code other than event handlers.”2
A workaround implemented in recently reviewed source code removed the Flush() call from the Dispose() method and introduced an IsFlushBeforeDisposeRequired() method. Then, all instances of the following code snippet:
using (var myService = new MyService(myStream))
{
await myService.WriteAsync("success");
}
were updated to the following:
using (var myService = new MyService(myStream))
{
await myService.WriteAsync("success");
if (myService.IsFlushBeforeDisposeRequired())
await myService.Flush();
}
Having to repeatedly check whether to call Flush() wherever MyService is disposed stinks! Simply calling Flush(), even if it is empty for derived classes of MyService also stinks!
System.IAsyncDisposable, introduced in .NET Standard 2.1 and .NET Core 3.0, addresses these code smells. async functions can be safely called from DisposeAsync(), which is a method of IAsyncDisposable, and boilerplate cleanup code can be avoided in using-blocks.
The following code is the above code updated to use IAsyncDisposable and works as expected by outputting “success” rather than an exception message:
using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
class MyService : IAsyncDisposable
{
private MemoryStream ms;
public MyService(MyStream myStream)
{
ms = myStream.GetStream();
}
public async Task WriteAsync(string s)
{
byte[] buf = System.Text.Encoding.ASCII.GetBytes(s);
await ms.WriteAsync(buf, 0, s.Length);
}
public async Task Flush()
{
try
{
await Task.Delay(50);
int length = (int)ms.Length;
byte[] buf = new byte[length];
ms.Seek(0, System.IO.SeekOrigin.Begin);
await ms.ReadAsync(buf, 0, length);
foreach (byte b in buf)
Console.Write((char)b);
Console.WriteLine("");
}
catch (Exception ex)
{
Console.WriteLine(ex.ToString());
}
Program.Done();
}
public async ValueTask DisposeAsync()
{
await Flush();
}
}
class MyStream : IDisposable
{
MemoryStream ms = new MemoryStream();
public MemoryStream GetStream()
{
return ms;
}
public void Dispose()
{
ms.Close();
}
}
class Program
{
private static EventWaitHandle ewh =
new EventWaitHandle(false, EventResetMode.ManualReset);
public static void Done()
{
ewh.Set();
}
public static async void Run()
{
using (var myStream = new MyStream())
{
await using (var myService = new MyService(myStream))
{
await myService.WriteAsync("success");
}
}
}
public static void Main(string[] args)
{
Run();
ewh.WaitOne();
}
}
Implementation of the IAsyncDisposable interface, the method signature of DisposeAsync(), and use of await using allows await Flush() to wait for completion before myStream is disposed. The updated code works as expected by outputting “success” instead of an exception message.
