Add remaining IAsyncDisposable implementations to CoreLib (dotnet/coreclr#20676)
authorStephen Toub <stoub@microsoft.com>
Sat, 3 Nov 2018 00:51:26 +0000 (20:51 -0400)
committerGitHub <noreply@github.com>
Sat, 3 Nov 2018 00:51:26 +0000 (20:51 -0400)
* Add remaining IAsyncDisposable implementations to CoreLib

* Address PR feedback

Commit migrated from https://github.com/dotnet/coreclr/commit/8b03dbeb90c81b8095b949b44553ff970e169851

src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs
src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs
src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs
src/libraries/System.Private.CoreLib/src/System/IO/TextWriter.cs

index d1a333f..dea1f03 100644 (file)
@@ -5,6 +5,7 @@
 using System.Text;
 using System.Diagnostics;
 using System.Buffers;
+using System.Threading.Tasks;
 
 namespace System.IO
 {
@@ -12,7 +13,7 @@ namespace System.IO
     // primitives to an arbitrary stream. A subclass can override methods to
     // give unique encodings.
     //
-    public class BinaryWriter : IDisposable
+    public class BinaryWriter : IDisposable, IAsyncDisposable
     {
         public static readonly BinaryWriter Null = new BinaryWriter();
 
@@ -87,6 +88,34 @@ namespace System.IO
             Dispose(true);
         }
 
+        public virtual ValueTask DisposeAsync()
+        {
+            try
+            {
+                if (GetType() == typeof(BinaryWriter))
+                {
+                    if (_leaveOpen)
+                    {
+                        return new ValueTask(OutStream.FlushAsync());
+                    }
+
+                    OutStream.Close();
+                }
+                else
+                {
+                    // Since this is a derived BinaryWriter, delegate to whatever logic
+                    // the derived implementation already has in Dispose.
+                    Dispose();
+                }
+
+                return default;
+            }
+            catch (Exception exc)
+            {
+                return new ValueTask(Task.FromException(exc));
+            }
+        }
+
         // Returns the stream associated with the writer. It flushes all pending
         // writes before returning. All subclasses should override Flush to
         // ensure that all buffered data is sent to the stream.
index ae4b709..5f3c55e 100644 (file)
@@ -269,6 +269,24 @@ namespace System.IO
             }
         }
 
+        public override ValueTask DisposeAsync()
+        {
+            // On Unix, we don't have any special support for async I/O, simply queueing writes
+            // rather than doing them synchronously.  As such, if we're "using async I/O" and we
+            // have something to flush, queue the call to Dispose, so that we end up queueing whatever
+            // write work happens to flush the buffer.  Otherwise, just delegate to the base implementation,
+            // which will synchronously invoke Dispose.  We don't need to factor in the current type
+            // as we're using the virtual Dispose either way, and therefore factoring in whatever
+            // override may already exist on a derived type.
+            if (_useAsyncIO && _writePos > 0)
+            {
+                return new ValueTask(Task.Factory.StartNew(s => ((FileStream)s).Dispose(), this,
+                    CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default));
+            }
+
+            return base.DisposeAsync();
+        }
+
         /// <summary>Flushes the OS buffer.  This does not flush the internal read/write buffer.</summary>
         private void FlushOSBuffer()
         {
index 4f8292b..91612dc 100644 (file)
@@ -226,24 +226,21 @@ namespace System.IO
             // finalized.
             try
             {
-                if (_fileHandle != null && !_fileHandle.IsClosed)
+                if (_fileHandle != null && !_fileHandle.IsClosed && _writePos > 0)
                 {
                     // Flush data to disk iff we were writing.  After 
                     // thinking about this, we also don't need to flush
                     // our read position, regardless of whether the handle
                     // was exposed to the user.  They probably would NOT 
                     // want us to do this.
-                    if (_writePos > 0)
+                    try
                     {
-                        try
-                        {
-                            FlushWriteBuffer(!disposing);
-                        }
-                        catch (Exception e) when (IsIoRelatedException(e) && !disposing)
-                        {
-                            // On finalization, ignore failures from trying to flush the write buffer,
-                            // e.g. if this stream is wrapping a pipe and the pipe is now broken.
-                        }
+                        FlushWriteBuffer(!disposing);
+                    }
+                    catch (Exception e) when (IsIoRelatedException(e) && !disposing)
+                    {
+                        // On finalization, ignore failures from trying to flush the write buffer,
+                        // e.g. if this stream is wrapping a pipe and the pipe is now broken.
                     }
                 }
             }
@@ -251,22 +248,46 @@ namespace System.IO
             {
                 if (_fileHandle != null && !_fileHandle.IsClosed)
                 {
-                    if (_fileHandle.ThreadPoolBinding != null)
-                        _fileHandle.ThreadPoolBinding.Dispose();
-
+                    _fileHandle.ThreadPoolBinding?.Dispose();
                     _fileHandle.Dispose();
                 }
 
-                if (_preallocatedOverlapped != null)
-                    _preallocatedOverlapped.Dispose();
-
+                _preallocatedOverlapped?.Dispose();
                 _canSeek = false;
 
                 // Don't set the buffer to null, to avoid a NullReferenceException
                 // when users have a race condition in their code (i.e. they call
                 // Close when calling another method on Stream like Read).
-                //_buffer = null;
-                base.Dispose(disposing);
+            }
+        }
+
+        public override ValueTask DisposeAsync() =>
+            GetType() == typeof(FileStream) ?
+                DisposeAsyncCore() :
+                base.DisposeAsync();
+
+        private async ValueTask DisposeAsyncCore()
+        {
+            // Same logic as in Dispose(), except with async counterparts.
+            // TODO: https://github.com/dotnet/corefx/issues/32837: FlushAsync does synchronous work.
+            try
+            {
+                if (_fileHandle != null && !_fileHandle.IsClosed && _writePos > 0)
+                {
+                    await FlushAsyncInternal(default).ConfigureAwait(false);
+                }
+            }
+            finally
+            {
+                if (_fileHandle != null && !_fileHandle.IsClosed)
+                {
+                    _fileHandle.ThreadPoolBinding?.Dispose();
+                    _fileHandle.Dispose();
+                }
+
+                _preallocatedOverlapped?.Dispose();
+                _canSeek = false;
+                GC.SuppressFinalize(this); // the handle is closed; nothing further for the finalizer to do
             }
         }
 
@@ -1544,6 +1565,7 @@ namespace System.IO
             if (_fileHandle.IsClosed)
                 throw Error.GetFileNotOpen();
 
+            // TODO: https://github.com/dotnet/corefx/issues/32837 (stop doing this synchronous work).
             // The always synchronous data transfer between the OS and the internal buffer is intentional 
             // because this is needed to allow concurrent async IO requests. Concurrent data transfer
             // between the OS and the internal buffer will result in race conditions. Since FlushWrite and
index 602900c..fe9f9fe 100644 (file)
@@ -25,7 +25,7 @@ using System.Threading.Tasks;
 
 namespace System.IO
 {
-    public abstract partial class Stream : MarshalByRefObject, IDisposable
+    public abstract partial class Stream : MarshalByRefObject, IDisposable, IAsyncDisposable
     {
         public static readonly Stream Null = new NullStream();
 
@@ -234,6 +234,19 @@ namespace System.IO
             // torn down.  This is the last code to run on cleanup for a stream.
         }
 
+        public virtual ValueTask DisposeAsync()
+        {
+            try
+            {
+                Dispose();
+                return default;
+            }
+            catch (Exception exc)
+            {
+                return new ValueTask(Task.FromException(exc));
+            }
+        }
+
         public abstract void Flush();
 
         public Task FlushAsync()
@@ -1202,6 +1215,12 @@ namespace System.IO
                 }
             }
 
+            public override ValueTask DisposeAsync()
+            {
+                lock (_stream)
+                    return _stream.DisposeAsync();
+            }
+
             public override void Flush()
             {
                 lock (_stream)
index 8d94ac6..d55eaae 100644 (file)
@@ -195,35 +195,62 @@ namespace System.IO
             }
             finally
             {
-                // Dispose of our resources if this StreamWriter is closable. 
-                // Note: Console.Out and other such non closable streamwriters should be left alone 
-                if (!LeaveOpen && _stream != null)
+                CloseStreamFromDispose(disposing);
+            }
+        }
+
+        private void CloseStreamFromDispose(bool disposing)
+        {
+            // Dispose of our resources if this StreamWriter is closable. 
+            if (!LeaveOpen && _stream != null)
+            {
+                try
                 {
-                    try
+                    // Attempt to close the stream even if there was an IO error from Flushing.
+                    // Note that Stream.Close() can potentially throw here (may or may not be
+                    // due to the same Flush error). In this case, we still need to ensure 
+                    // cleaning up internal resources, hence the finally block.  
+                    if (disposing)
                     {
-                        // Attempt to close the stream even if there was an IO error from Flushing.
-                        // Note that Stream.Close() can potentially throw here (may or may not be
-                        // due to the same Flush error). In this case, we still need to ensure 
-                        // cleaning up internal resources, hence the finally block.  
-                        if (disposing)
-                        {
-                            _stream.Close();
-                        }
-                    }
-                    finally
-                    {
-                        _stream = null;
-                        _byteBuffer = null;
-                        _charBuffer = null;
-                        _encoding = null;
-                        _encoder = null;
-                        _charLen = 0;
-                        base.Dispose(disposing);
+                        _stream.Close();
                     }
                 }
+                finally
+                {
+                    _stream = null;
+                    _byteBuffer = null;
+                    _charBuffer = null;
+                    _encoding = null;
+                    _encoder = null;
+                    _charLen = 0;
+                    base.Dispose(disposing);
+                }
             }
         }
 
+        public override ValueTask DisposeAsync() =>
+            GetType() != typeof(StreamWriter) ?
+                base.DisposeAsync() :
+                DisposeAsyncCore();
+
+        private async ValueTask DisposeAsyncCore()
+        {
+            // Same logic as in Dispose(), but with async flushing.
+            Debug.Assert(GetType() == typeof(StreamWriter));
+            try
+            {
+                if (_stream != null)
+                {
+                    await FlushAsync().ConfigureAwait(false);
+                }
+            }
+            finally
+            {
+                CloseStreamFromDispose(disposing: true);
+            }
+            GC.SuppressFinalize(this);
+        }
+
         public override void Flush()
         {
             CheckAsyncTaskInProgress();
index 99f99b6..c164027 100644 (file)
@@ -18,7 +18,7 @@ namespace System.IO
     //
     // This class is intended for character output, not bytes.
     // There are methods on the Stream class for writing bytes.
-    public abstract partial class TextWriter : MarshalByRefObject, IDisposable
+    public abstract partial class TextWriter : MarshalByRefObject, IDisposable, IAsyncDisposable
     {
         public static readonly TextWriter Null = new NullTextWriter();
 
@@ -79,6 +79,19 @@ namespace System.IO
             GC.SuppressFinalize(this);
         }
 
+        public virtual ValueTask DisposeAsync()
+        {
+            try
+            {
+                Dispose();
+                return default;
+            }
+            catch (Exception exc)
+            {
+                return new ValueTask(Task.FromException(exc));
+            }
+        }
+
         // Clears all buffers for this TextWriter and causes any buffered data to be
         // written to the underlying device. This default method is empty, but
         // descendant classes can override the method to provide the appropriate
@@ -774,6 +787,14 @@ namespace System.IO
                     ((IDisposable)_out).Dispose();
             }
 
+            // [MethodImpl(MethodImplOptions.Synchronized)]
+            public override ValueTask DisposeAsync()
+            {
+                // TODO: https://github.com/dotnet/coreclr/issues/20499
+                // Manual synchronization should be replaced by Synchronized for consistency.
+                lock (this) return _out.DisposeAsync();
+            }
+
             [MethodImpl(MethodImplOptions.Synchronized)]
             public override void Flush() => _out.Flush();