Temporary file created by HybridMemoryStream in Transposer not deleted

[mjmckp]

The HybridMemoryStream created in the Transposer

machinelearning/src/Microsoft.ML.Data/DataView/Transposer.cs

Line 173 in c023727

var stream = new HybridMemoryStream();

is closed, but never disposed. This is because the Dispose implementation of the BinaryLoader here

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Binary/BinaryLoader.cs

Line 1184 in c023727

_reader.Dispose();

only calls Dispose on the System.IO.BinaryReader it contains, which in turn only calls Close on HybridMemoryStream, but not Dispose (see https://referencesource.microsoft.com/#mscorlib/system/io/binaryreader.cs,91). Therefore, the deletion of the temporary file here

machinelearning/src/Microsoft.ML.Core/Utilities/HybridMemoryStream.cs

Line 127 in c023727

File.Delete(_overflowPath);

is never reached.

This is causing the temporary directory on my PC to fill up all the available space on the disk.

[TomFinley]

D'oh, thanks for finding this @mjmckp . :( I'm rather surprised that the System.IO.BinaryReader dispose does not call dispose on the stream. My expectation, which may be wrong, is that if something implements IDisposable that any objects it "owns" that are also IDisposable should be likewise disposed. But, possibly my expectations are incorrect.

Generally from what I read here:
https://github.com/dotnet/corefx/blob/55f2293ddda3c972fcc2d94915b03bc8556e8c9b/src/Common/src/CoreLib/System/IO/Stream.cs#L211-L223
I think the idea is that Stream.Close overrides should call base.Close, and I notice that we're not doing that here and in one other case. If we do that perhaps that will resolve the issue.

[glebuk]

I suggest we use belt and suspenders approach. Even with proper Dispose() code, if the process is killed due to timeout, error, or user action, the file will remain. There exists a DeleteOnClose option that instructs the OS to delete this file even in the process is suddenly killed without disposers running their course.

Suggest we update the IFileHandle.cs to do something like this:

if (_autoDelete)
    _streamWrite = new FileStream(_fullPath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite, 4096, FileOptions.DeleteOnClose);