From 05675566b6259438df54c12b503714cad4bf2d2c Mon Sep 17 00:00:00 2001 From: Tom Finley Date: Mon, 18 Jun 2018 09:42:18 -0700 Subject: [PATCH 1/4] Subclasses of `Stream` now have `Close` call `base.Close` to ensure disposal. --- .../Utilities/HybridMemoryStream.cs | 28 +++++++++---------- .../Utilities/TextReaderStream.cs | 27 +++++------------- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs index 8825369455..ead61426a7 100644 --- a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs +++ b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs @@ -25,20 +25,19 @@ public sealed class HybridMemoryStream : Stream private bool _disposed; - private Stream MyStream { get { return _memStream ?? _overflowStream; } } + private Stream MyStream => _memStream ?? _overflowStream; - private bool IsMemory { get { return _memStream != null; } } + private bool IsMemory => _memStream != null; - public override long Position - { - get { return MyStream.Position; } - set { Seek(value, SeekOrigin.Begin); } + public override long Position { + get => MyStream.Position; + set => Seek(value, SeekOrigin.Begin); } - public override long Length { get { return MyStream.Length; } } - public override bool CanWrite { get { return MyStream.CanWrite; } } - public override bool CanSeek { get { return MyStream.CanSeek; } } - public override bool CanRead { get { return MyStream.CanRead; } } + public override long Length => MyStream.Length; + public override bool CanWrite => MyStream.CanWrite; + public override bool CanSeek => MyStream.CanSeek; + public override bool CanRead => MyStream.CanRead; /// /// Constructs an initially empty read-write stream. Once the number of @@ -129,21 +128,22 @@ protected override void Dispose(bool disposing) } _disposed = true; AssertInvariants(); + base.Dispose(disposing); } } public override void Close() { AssertInvariants(); - if (MyStream != null) - MyStream.Close(); + MyStream?.Close(); + // The base Stream class Close will call Dispose(bool). + base.Close(); } public override void Flush() { AssertInvariants(); - if (MyStream != null) - MyStream.Flush(); + MyStream?.Flush(); AssertInvariants(); } diff --git a/src/Microsoft.ML.Core/Utilities/TextReaderStream.cs b/src/Microsoft.ML.Core/Utilities/TextReaderStream.cs index ab83bf40f0..5c05275ba7 100644 --- a/src/Microsoft.ML.Core/Utilities/TextReaderStream.cs +++ b/src/Microsoft.ML.Core/Utilities/TextReaderStream.cs @@ -14,7 +14,7 @@ namespace Microsoft.ML.Runtime.Internal.Utilities /// compensates by inserting \n line feed characters at the end of every /// input line, including the last one. /// - public class TextReaderStream : Stream + public sealed class TextReaderStream : Stream { private readonly TextReader _baseReader; private readonly Encoding _encoding; @@ -38,19 +38,11 @@ public class TextReaderStream : Stream public override bool CanWrite => false; public override long Length - { - get - { - throw Contracts.ExceptNotSupp("Stream cannot determine length."); - } - } + => throw Contracts.ExceptNotSupp("Stream cannot determine length."); public override long Position { - get - { - return _position; - } + get => _position; set { if (value != Position) @@ -96,6 +88,7 @@ public override void Close() protected override void Dispose(bool disposing) { _baseReader.Dispose(); + base.Dispose(disposing); } public override void Flush() @@ -182,18 +175,12 @@ public override int ReadByte() } public override long Seek(long offset, SeekOrigin origin) - { - throw Contracts.ExceptNotSupp("Stream cannot seek."); - } + => throw Contracts.ExceptNotSupp("Stream cannot seek."); public override void Write(byte[] buffer, int offset, int count) - { - throw Contracts.ExceptNotSupp("Stream is not writable."); - } + => throw Contracts.ExceptNotSupp("Stream is not writable."); public override void SetLength(long value) - { - throw Contracts.ExceptNotSupp("Stream is not writable."); - } + => throw Contracts.ExceptNotSupp("Stream is not writable."); } } \ No newline at end of file From 8d02e4ef82fd3e8ced8afff328355c809dc2ee6c Mon Sep 17 00:00:00 2001 From: Tom Finley Date: Mon, 18 Jun 2018 11:26:33 -0700 Subject: [PATCH 2/4] * Add DeleteOnClose. * Remove explicit delete of file. * Remove explicit close of substream. --- src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs index ead61426a7..2b31dc459d 100644 --- a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs +++ b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs @@ -123,7 +123,6 @@ protected override void Dispose(bool disposing) _overflowStream = null; overflow.Dispose(); Contracts.AssertValue(_overflowPath); - File.Delete(_overflowPath); _overflowPath = null; } _disposed = true; @@ -135,7 +134,6 @@ protected override void Dispose(bool disposing) public override void Close() { AssertInvariants(); - MyStream?.Close(); // The base Stream class Close will call Dispose(bool). base.Close(); } @@ -166,7 +164,8 @@ private void EnsureOverflow() Contracts.Assert(_overflowPath == null); _overflowPath = Path.GetTempFileName(); - _overflowStream = new FileStream(_overflowPath, FileMode.Open, FileAccess.ReadWrite); + _overflowStream = new FileStream(_overflowPath, FileMode.Open, FileAccess.ReadWrite, + FileShare.None, bufferSize: 4096, FileOptions.DeleteOnClose); // The documentation is not clear on this point, but the source code for // memory stream makes clear that this buffer is exposable for a memory From 982930e773fa384c2495375c4222c9f2673dfa1d Mon Sep 17 00:00:00 2001 From: Tom Finley Date: Mon, 18 Jun 2018 11:30:24 -0700 Subject: [PATCH 3/4] Since no longer deleting explicitly, no longer need `_overflowPath` member. --- src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs index 2b31dc459d..ca31636c09 100644 --- a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs +++ b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs @@ -19,7 +19,6 @@ public sealed class HybridMemoryStream : Stream { private MemoryStream _memStream; private Stream _overflowStream; - private string _overflowPath; private readonly int _overflowBoundary; private const int _defaultMaxLen = 1 << 30; @@ -162,9 +161,8 @@ private void EnsureOverflow() // been closed. Contracts.Check(_memStream.CanRead, "attempt to perform operation on closed stream"); - Contracts.Assert(_overflowPath == null); - _overflowPath = Path.GetTempFileName(); - _overflowStream = new FileStream(_overflowPath, FileMode.Open, FileAccess.ReadWrite, + string overflowPath = Path.GetTempFileName(); + _overflowStream = new FileStream(overflowPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None, bufferSize: 4096, FileOptions.DeleteOnClose); // The documentation is not clear on this point, but the source code for From 87be344ab041446ee3841b1893392faf20d04d8a Mon Sep 17 00:00:00 2001 From: Tom Finley Date: Mon, 18 Jun 2018 11:44:24 -0700 Subject: [PATCH 4/4] Bro, do you even build? --- src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs index ca31636c09..73b4c4a828 100644 --- a/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs +++ b/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs @@ -121,8 +121,6 @@ protected override void Dispose(bool disposing) var overflow = _overflowStream; _overflowStream = null; overflow.Dispose(); - Contracts.AssertValue(_overflowPath); - _overflowPath = null; } _disposed = true; AssertInvariants();