dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

API Proposal: Create a Temporary Directory class in System.IO #2048

Open JeremyKuhne opened 4 years ago

JeremyKuhne commented 4 years ago

We need a more official, general purpose TempFileCollection that lives in System.IO that allows you to:

  1. Create a temp directory that you can clean via disposal.
  2. Generate configurable filenames.
  3. Generate temp FileStream instances (which is more dependable than taking creating your own from a string, and it is also encourages writing more secure code).
  4. Potentially have the ability to clean orphaned temp directories (say, after an app crash). I've written a few of these, it is just might not be worth complicating the API here to do so. (Might be better as a second class.)

We could extend TempFileCollection, but it is purpose built for CodeDom and isn't very discoverable. I'm not very keen on pushing that one forward.

dotnet/corefx#41961, dotnet/corefx#22972, dotnet/corefx#24001 cover functionality requests that would be handled by this.

drmcclelland commented 3 years ago

This would be extremely useful, especially if has an overload to create a temp file in any directory, as suggested by @danmoseley in https://github.com/dotnet/runtime/issues/40414#issuecomment-669488133

static File | createTempFile(String prefix, String suffix) Creates an empty file in the default temporary-file directory, using the given prefix and suffix to generate its name. static File | createTempFile(String prefix, String suffix, File directory) Creates a new empty file in the specified directory, using the given prefix and suffix strings to generate its name.

danmoseley commented 3 years ago

Just clarifying the quote above was from Java. To make progress here, someone needs to create a formal API proposal (whether it be a type or just new methods) using the standard format. It can be pasted here. Then we can all discuss shape/behavior.

iSazonov commented 3 years ago

/cc @carlossanlop Related issues:

Design considerations:

For 1. We need to create a subdirectory in common temporary directory with the application name (as default) and unique session number. For 2. Most useful is to have configurable file extensions (".tmp" as default). If we implement p.1 we have no need to complicate the API with support configurable file prefixes. See follow TempFileCollection use case: https://github.com/dotnet/runtime/blob/ebba45155160c7056faf1675448af1b75e9f4a48/src/libraries/System.CodeDom/src/System/CodeDom/Compiler/CodeCompiler.cs#L185-L191 For 3. There are a lot of options for Stream class. We could implement common case with strongly locked temp file and define a delegate for other scenarios. See follow TempFileCollection use case: https://github.com/dotnet/runtime/blob/ebba45155160c7056faf1675448af1b75e9f4a48/src/libraries/System.CodeDom/src/System/CodeDom/Compiler/Executor.cs#L50-L54 https://github.com/dotnet/runtime/blob/ebba45155160c7056faf1675448af1b75e9f4a48/src/libraries/System.CodeDom/src/System/CodeDom/Compiler/Executor.cs#L15-L16

For 4. It is out of my design since I believe it is better implement by OS means.

  1. Temporary files imply their automatic deletion after use.
  2. Temporary files can be created for a short time (create-use-delete) or they can be accumulated for joint processing (with automatic deletion at the end) - see links above for use examples of the TempFileCollection in System.CodeDom.Compiler
  3. There was a concern about security on Unix https://github.com/dotnet/runtime/issues/23538#issuecomment-329363372. If we implement p.1 all files will be in OS created temporary directory. If we accept the temp directory name from an user the user must set right mode/permissions on the directory. Also we could utilize Path.GetTempFileName() but it is not support custom file extensions - we could add this in the API proposal if needed.
  4. We could benefit from FileAttributes.Temporary

Proposal

Implementation https://github.com/dotnet/runtime/compare/main...iSazonov:tempdir

namespace System.IO
{
    public sealed class TemporaryDirectory : IDisposable
    {
        public TemporaryDirectory() { }
        public TemporaryDirectory(string tempDir) { }
        public TemporaryDirectory(bool keepFiles) { }
        public TemporaryDirectory(string tempDir, bool storeFileNames) { }
        void System.IDisposable.Dispose() { }
        public void Dispose(bool disposing) { }
        ~TemporaryDirectory() { }
        public static string TemporaryDirectoryPrefix { get { throw null; } set { } }
        public static FileStream CreateTempFileStream(bool keepFile = false) { throw null; }
        public static FileStream CreateTempFileStream(string fileExtension, bool keepFile = false) { throw null; }
        public static FileStream CreateTempFileStream(CreateTempFileStreamFunc func, bool keepFile = false) { throw null; }
        public static FileStream CreateTempFileStream(CreateTempFileStreamFunc func, string fileExtension, bool keepFile = false) { throw null; }
        public delegate FileStream CreateTempFileStreamFunc(string fileName, bool keepFile);
        public FileStream GetTempFileNameStream() { throw null; }
        public FileStream GetTempFileNameStream(bool keepFile) { throw null; }
        public FileStream GetTempFileNameStream(string fileExtension) { throw null; }
        public FileStream GetTempFileNameStream(string fileExtension, bool keepFile) { throw null; }
        public FileStream GetTempFileNameStream(CreateTempFileStreamFunc func) { throw null; }
        public FileStream GetTempFileNameStream(CreateTempFileStreamFunc func, bool keepFile) { throw null; }
        public FileStream GetTempFileNameStream(CreateTempFileStreamFunc func, string fileExtension) { throw null; }
        public FileStream GetTempFileNameStream(CreateTempFileStreamFunc func, string fileExtension, bool keepFile) { throw null; }
        public string GetTempFileName() { throw null; }
        public string GetTempFileName(bool storeFileName) { throw null; }
        public string GetTempFileName(string fileExtension) { throw null; }
        public string GetTempFileName(string fileExtension, bool storeFileName) { throw null; }
        public string BaseTemporaryDirectory { get { throw null; } }
        public bool StoreFileNames { get { throw null; } }
        public void Clear() { }
        public IReadOnlyList<string> FileNameList { get { throw null; } }
    }
}
drmcclelland commented 3 years ago

The proposed implementation by @iSazonov looks great!

tmds commented 3 years ago

The simplest API is a counterpart to Path.GetTempFileName:

public static class Path
{
    /// <summary>
    /// Creates a uniquely named, empty directory on disk and returns the full path of that directory. 
    /// </summary>
    public static string GetTempDirectoryName();
}

Maybe we can start with that?

The proposed TemporaryDirectory adds additional functionality on top.

On Unix, like mkdtemp, the directory should be created with permissions 0700 so other users can not delete/modify files in this directory.

iSazonov commented 3 years ago

Maybe we can start with that?

It was. It was deemed not as useful as addressing more real-world scenarios, which was done. Several issues have been closed in favor of that.

tmds commented 3 years ago

@iSazonov is your point that it is not solving all use-cases? or that it is too low level?

iSazonov commented 3 years ago

@tmds I think no needs to have just another low level API because we already have GetTempFileName() and GetTempPath(). Using the API developers can implement any scenario.

tmds commented 3 years ago

I think no needs to have just another low level API because we already have GetTempFileName() and GetTempPath(). Using the API developers can implement any scenario.

Using these APIs it's not possible to set the mode to 0700 which is needed to create a temp folder on Unix that can not be read/modified by others.

The File APIs have several ways to do the same thing. The static functions provided allow users to build something themselves without forcing them into certain patterns.

So I think it's useful to have:

public static class Path
{
    /// <summary>
    /// Creates a uniquely named, empty directory on disk and returns the full path of that directory. 
    /// </summary>
    public static string GetTempDirectoryName();
}

API feedback for TemporaryDirectory:

iSazonov commented 3 years ago

I think no needs to have just another low level API because we already have GetTempFileName() and GetTempPath(). Using the API developers can implement any scenario.

Using these APIs it's not possible to set the mode to 0700 which is needed to create a temp folder on Unix that can not be read/modified by others.

GetTempFileName() does this for you on Unix. GetTempPath() returns that OS defines and users should care about security - this is why the API proposal was created: introduce new safe API for common scenarios.

So I think it's useful to have:

You can reopen #31243. But before ask yourself that scenarios you want address and aren't all of these scenarios covered by the API proposal.

tmds commented 3 years ago

GetTempFileName() does this for you on Unix.

Yes, for files. There is no API for directories.

You can reopen #31243. But before ask yourself that scenarios you want address and aren't all of these scenarios covered by the API proposal.

I think it is meaningful to have a method next to GetTempFileName for directories independent of whether there is a TemporaryDirectory class that provides a larger set of features.

@JeremyKuhne should weigh in here, since he closed #31243. @danmoseley do you have some thoughts about this?

iSazonov commented 3 years ago

I think it is meaningful to have a method next to GetTempFileName for directories independent of whether there is a TemporaryDirectory class that provides a larger set of features.

To approve the API we need know scenarios the API covers. Nobody needs a temporary directory per se. The next step after creating temp directory will always be to create a temporary file or files - TemporaryDirectory class covers a lot of similar scenarios.

tmds commented 3 years ago

The next step after creating temp directory will always be to create a temporary file or files

It can also be non-temporary files in a directory that happens to be temporary. The files don't need to know, so they don't need to be created using TemporaryDirectory. Temporary files can also be created in directories which are not temporary, so it's unexpected to have to use TemporaryDirectory for that.

iSazonov commented 3 years ago

It can also be non-temporary files in a directory that happens to be temporary. The files don't need to know, so they don't need to be created using TemporaryDirectory.

Non-temporary files in a temp directory? This seems absurd. Can you specify a real project on GitHub where this is used?

Temporary files can also be created in directories which are not temporary, so it's unexpected to have to use TemporaryDirectory for that.

TemporaryDirectory covers such scenarios too. Users can initialize TemporaryDirectory with existing non-temporary directory and have temp files there with auto removal.

tmds commented 3 years ago

Non-temporary files in a temp directory? This seems absurd. Can you specify a real project on GitHub where this is used?

I mean: if the responsibility to delete the files lies with the temporary directory, there is nothing special about the files.

Users can initialize TemporaryDirectory with existing non-temporary directory.

The naming fails here. The referenced TempFileCollection is a better name.

JeremyKuhne commented 3 years ago

I think it is meaningful to have a method next to GetTempFileName for directories independent of whether there is a TemporaryDirectory class that provides a larger set of features.

I'm not fundamentally opposed to it. Note, however, that I no longer am working on System.IO.

tmds commented 3 years ago

Temporary means two different things:

I think we should keep these separate.

APIs for deleting using IDisposable:

// Deletes files and directories on Dispose.
sealed class TemporaryFileCollection : IDisposable
{
    public TempFileCollection();
    public void AddFile(string path);
    public void AddDirectory(string path);
    public void DeleteAll(); // throws AggregateException
    public bool TryDeleteAll();
    public void Dispose() => TryDeleteAll();
}

// Deletes a file on Dispose.
// (may be a struct?)
sealed class TemporaryFile : IDisposable
{
    public TemporaryFile(string path);
    public string Path { get; }
}

// Deletes a directory on Dispose.
// (may be a struct?)
sealed class TemporaryDirectory : IDisposable
{
    public TemporaryDirectory(string path);
    public string Path { get; }
}

APIs for randomly naming:

static class Directory
{
    // Creates a uniquely named directory with access restricted to the current user.
    public static string CreateTempDirectory();
}

static class File
{
    // Creates a uniquely named file.
    public static FileStream CreateTempFile(string directory, string extension = "tmp", FileAccess access = FileAccess.Write, FileShare share = FileShare.None, int bufferSize = 4096, FileOptions options = FileOptions.None);
}

Example:

using TemporaryDirectory tmpDir = new(Directory.CreateTempDirectory());
using FileStream file = File.CreateTempFile(tmpDir.Path);
maxkatz6 commented 3 years ago

On windows for both UWP and Win32 apps distributed with MSIX packaging system it makes sense to use "%localappdata%/Packages/%PackageName%/TempState" as a temp folder location instead of global OS "Temp" folder heap. https://docs.microsoft.com/en-us/uwp/api/windows.storage.applicationdata.temporaryfolder?view=winrt-20348

It also does not requires any special permissions.

jgilbert2017 commented 10 months ago

it would be nice to have GetTempFileName(string ext) but it looks like an issue from 2017(!) was closed and redirected to this one. https://github.com/dotnet/runtime/issues/23538

any chance of reviving this?

the proposed API seems.... a bit extensive. can we maybe shoot for an easier target that could be introduced for net9? GetTempFileName(string ext) seems like pretty low hanging fruit. thanks.