Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JeremyKuhne opened this issue Jan 22, 2020 · 19 comments
Open

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

JeremyKuhne opened this issue Jan 22, 2020 · 19 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO
Milestone

Comments

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Jan 22, 2020

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.

@JeremyKuhne JeremyKuhne added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO labels Jan 22, 2020
@JeremyKuhne JeremyKuhne added this to the 5.0 milestone Jan 22, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 22, 2020
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Jan 22, 2020
@carlossanlop carlossanlop added this to To do in System.IO - File system via automation Mar 6, 2020
@carlossanlop carlossanlop modified the milestones: 5.0.0, Future Jun 18, 2020
@drmcclelland
Copy link

This would be extremely useful, especially if has an overload to create a temp file in any directory, as suggested by @danmoseley in #40414 (comment)

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
Copy link
Member

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
Copy link
Contributor

iSazonov commented May 25, 2021

/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:

filenames[i] = options.TempFiles.AddExtension(i + FileExtension);
using (var fs = new FileStream(filenames[i], FileMode.Create, FileAccess.Write, FileShare.Read))
using (var sw = new StreamWriter(fs, Encoding.UTF8))
{
((ICodeGenerator)this).GenerateCodeFromCompileUnit(ea[i], sw, Options);
sw.Flush();
}

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:
using (var outputWriter = new StreamWriter(CreateInheritedFile(outputName), Encoding.UTF8))
using (var errorWriter = new StreamWriter(CreateInheritedFile(errorName), Encoding.UTF8))
{
// Output the command line...
outputWriter.Write(currentDir);

private static FileStream CreateInheritedFile(string file) =>
new FileStream(file, FileMode.CreateNew, FileAccess.Write, FileShare.Read | FileShare.Inheritable);

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 Enhance API to work with temporary files and directories #23538 (comment). 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 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
Copy link

The proposed implementation by @iSazonov looks great!

@tmds
Copy link
Member

tmds commented Jul 1, 2021

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
Copy link
Contributor

iSazonov commented Jul 1, 2021

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
Copy link
Member

tmds commented Jul 1, 2021

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

@iSazonov
Copy link
Contributor

iSazonov commented Jul 2, 2021

@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
Copy link
Member

tmds commented Jul 2, 2021

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:

  • The IDisposable pattern is a nice to delete the directory when the instance goes out of scope.
  • I think the finalizer may be omitted.
  • The file system tracks files also. Are there enough use-cases that justify the additional API surface for tracking the created files?
  • An alternative for the methods that return a FileStream may be to add it on the static File class next to the other Open* methods.
public static class File
{
    public static FileStream OpenTempFile(string directory, string extension = "tmp", FileMode mode = FileMode.CreateNew, FileAccess access = FileAccess.Write, FileShare share = FileShare.None, int bufferSize = 4096, FileOptions options= FileOptions.None);
}

@iSazonov
Copy link
Contributor

iSazonov commented Jul 2, 2021

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
Copy link
Member

tmds commented Jul 2, 2021

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
Copy link
Contributor

iSazonov commented Jul 2, 2021

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
Copy link
Member

tmds commented Jul 2, 2021

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
Copy link
Contributor

iSazonov commented Jul 2, 2021

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
Copy link
Member

tmds commented Jul 2, 2021

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
Copy link
Member Author

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
Copy link
Member

tmds commented Jul 10, 2021

Temporary means two different things:

  • creating randomly named (+permissions)
  • deleted

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
Copy link
Contributor

maxkatz6 commented Jul 26, 2021

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
Copy link

it would be nice to have GetTempFileName(string ext) but it looks like an issue from 2017(!) was closed and redirected to this one.
#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO
Projects
Development

No branches or pull requests

10 participants