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]: Creating an empty temporary directory #72881

Open
eerhardt opened this issue Jul 26, 2022 · 26 comments
Open

[API Proposal]: Creating an empty temporary directory #72881

eerhardt opened this issue Jul 26, 2022 · 26 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.IO needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Jul 26, 2022

Background and motivation

When dealing with temporary files, especially with more than one, we would like to create our own folder and place them inside. There are several ways to achieve this but for convenience and for completion of the existing Path.GetTempFileName there should be an official API for this.

This has been requested a few times before (#2048, #23538, #31243). #2048 is a large higher-level API proposal for an object model of working with temporary files. To make forward progress here, for 7.0 we should add a simple API that creates a new, empty temp directory. This would correspond with other Unix POSIX work we've been doing in 7.0 (#68770, #67837).

On Unix, this would use mkdtemp, just like how Path.GetTempFileName uses mkstemps. On Windows, this API would generate a new random directory name (possibly using Guid.NewGuid) guaranteeing that the new directory was freshly created.

API Proposal

namespace System.IO;

public static class Path
{
    public static string GetTempPath();
    public static string GetTempFileName();

+    /// <summary>
+    /// Creates a uniquely named, empty directory and returns the full path of that directory. 
+    /// </summary>
+    public static string CreateTemporaryDirectory();
}

API Usage

string tempDirectory = Path.CreateTemporaryDirectory();

File.WriteAllText(Path.Combine(tempDirectory, "file1.txt"), "First file");
File.WriteAllText(Path.Combine(tempDirectory, "file2.txt"), "Second file");

// do something with file1.txt and file2.txt

Directory.Delete(tempDirectory, recursive: true);

Open Questions

  1. Should CreateTemporaryDirectory optionally take a string prefix? mkdtemp allows for prefixes. It would allow for libraries/components to "group" their temp directories together in case users need to find them for some reason.

  2. Should it return DirectoryInfo instead of string? No other methods in Path deal with FileInfo and DirectoryInfo.

  3. We could put it on Directory.CreateTemporaryDirectory() instead of Path - but Path is where GetTempPath and GetTempFileName is. It would be hard to justify putting it in a separate class from the rest of the "temp" methods.

Alternative Designs

Alternatively we could create a full blown TemporaryDirectory class as proposed in #2048. However, that API can be built on top of this one in the future. Adding this lower-level API separately allows for new code to start using it now.

Risks

No response

@eerhardt eerhardt added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO labels Jul 26, 2022
@eerhardt eerhardt added this to the 7.0.0 milestone Jul 26, 2022
@ghost
Copy link

ghost commented Jul 26, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

When dealing with temporary files, especially with more than one, we would like to create our own folder and place them inside. There are several ways to achieve this but for convenience and for completion of the existing Path.GetTempFileName there should be an official API for this.

This has been requested a few times before (#2048, #23538, #31243). #2048 is a large higher-level API proposal for an object model of working with temporary files. To make forward progress here, for 7.0 we should add a simple API that creates a new, empty temp directory. This would correspond with other Unix POSIX work we've been doing in 7.0 (#68770, #67837).

On Unix, this would use mkdtemp, just like how Path.GetTempFileName uses mkstemps. On Windows, this API would generate a new random directory name (possibly using Guid.NewGuid) guaranteeing that the new directory was freshly created.

API Proposal

namespace System.IO;

public static class Path
{
    public static string GetTempPath();
    public static string GetTempFileName();

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

API Usage

string tempDirectory = Path.GetTempDirectoryName();

File.WriteAllText(Path.Combine(tempDirectory, "file1.txt"), "First file");
File.WriteAllText(Path.Combine(tempDirectory, "file2.txt"), "Second file");

// do something with file1.txt and file2.txt

Directory.Delete(tempDirectory, recursive: true);

Alternative Designs

Alternatively we could create a full blown TemporaryDirectory class as proposed in #2048. However, that API can be built on top of this one in the future. Adding this lower-level API separately allows for new code to start using it now.

Risks

No response

Author: eerhardt
Assignees: -
Labels:

api-suggestion, area-System.IO

Milestone: 7.0.0

@eerhardt eerhardt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 26, 2022
@AaronRobinsonMSFT

This comment was marked as off-topic.

@stephentoub
Copy link
Member

stephentoub commented Jul 26, 2022

I wonder if the similarity between Path.GetTempPath() and Path.GetTempDirectoryName() would lead someone to call the latter when they'd intended to call the former (e.g. "Hmm, I need to put something in temp... what's the name of the temp directory? Oh, look, I'll call Path.GetTempDirectoryName to get it"). Maybe that'd be a good thing, except it also wouldn't be idempotent, and so calling it multiple times would likely lead them to a bad place. I think offline you'd suggested a Create prefix... that might be better, even though it diverges from GetTempFileName.

@Jozkee
Copy link
Member

Jozkee commented Jul 26, 2022

On Windows, this API would generate a new random directory name (possibly using Guid.NewGuid)

I guess you can use Path.GetRandomFileName() rather than a GUID.

@eerhardt
Copy link
Member Author

I think offline you'd suggested a Create prefix... that might be better, even though it diverges from GetTempFileName.

I went back and forth on using Create, or being consistent with GetTempFileName. I think your comment pushed me to be fully behind using Create in the name. It is just the better name, even if it is inconsistent with GetTempFileName.

I guess you can use Path.GetRandomFileName() rather than a GUID.

It would be shorter - GUIDs are 32 characters long, GetRandomFileName is 12 (8 + . + 3). So the odds of conflicting are higher, but that just means we will need to run the loop more often, even though it is pretty unlikely to conflict.


One other question that has come up is: should this return a DirectoryInfo, like Directory.CreateDirectory does. Nothing else in the Path class does, it just talks in terms of string. But I wanted to raise the question.

Another alternative is we could put it on Directory.CreateTemporaryDirectory() instead of Path - but Path is where GetTempPath and GetTempFileName is. It would be hard to justify putting it in a separate class from the rest of the "temp" methods.

@danmoseley danmoseley added the blocking Marks issues that we want to fast track in order to unblock other important work label Jul 27, 2022
@Clockwork-Muse
Copy link
Contributor

empty directory on disk

Linux especially (unsure about Windows) the default temp directory is a tmpfs filesystem, which is ram-backed (although pages out to disk as required). We should probably avoid mentioning anything about "disk" in this context, because presumably it's going to go wherever the currently configured temp directory is.

@deeprobin
Copy link
Contributor

deeprobin commented Jul 27, 2022

If we use a Guid, I think NewGuid is a wrong option.

NewGuid tries to generate a cryptographically secure random Guid.
Performance-wise this would not be fatal compared to a "normal" random, but I think with such IO topics you have to keep the performance high.
E.g. if many temporary folders have to be created.

I created several times ago a proposal for creating a Non cryptographical Guid for ex. #65736.
See also #22721

@eerhardt
Copy link
Member Author

eerhardt commented Jul 27, 2022

I guess you can use Path.GetRandomFileName() rather than a GUID.

If we use a Guid, I think NewGuid is a wrong option.

On my windows machine, I'm not seeing much of a performance difference in the following benchmark between doing Guid.NewGuid and Path.GetRandomFileName:

[MemoryDiagnoser]
public class CreateTemporaryDirectoryBenchmark
{
    [Benchmark]
    public void CreateTemporaryDirectory()
    {
        var path = Path.CreateTemporaryDirectory();
        Directory.Delete(path);
    }
}
Method Toolchain Mean Error StdDev Ratio RatioSD Allocated
CreateTemporaryDirectory guid 212.3 us 4.19 us 4.30 us 1.00 0.00 648 B
CreateTemporaryDirectory random 214.7 us 10.64 us 11.39 us 1.01 0.05 568 B

The extra allocation is due to Guid being 32 characters vs random file name being only 12.

You can see the implementation difference between these two in: 028637e

@terrajobst
Copy link
Member

terrajobst commented Aug 2, 2022

  • We believe Path.GetTempFileName() was a mistake (should have been called Path.CreateTempFileName())
    • Not bad enough to obsolete, but let's at least hide it
  • We'd prefer dedicated APIs on File and Directory
    • We believe customizing the prefix and extension (suffix) for files is desirable.
    • We should disallow path separators for both prefix and suffix
namespace System.IO;

public static class Path
{
    // Existing:
    // public static string GetTempPath();
    // public static string GetTempFileName();

    [EditorBrowsable(EditorBrowsableState.Never)]
    public static string GetTempFileName();
}

public partial class File
{
    public static FileInfo CreateTempFile(string? prefix = null, string? suffix = null);
}

public partial class Directory
{
    public static DirectoryInfo CreateTempSubdirectory(string? prefix = null);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 2, 2022
@Clockwork-Muse
Copy link
Contributor

In python temp files/directories are context objects, you can do the equivalent of:

using(var tmp = File.CreateAndOpenTempFile(FileMode.Create))
{
    DoSomething(tmp);
}

... and the objects are automatically cleaned up afterwards. Of course, doing that also auto-opens the file as well...

Should something like this be considered? Or this is just the base functionality, and that's a feature that should maybe be considered later.

@eerhardt
Copy link
Member Author

eerhardt commented Aug 2, 2022

... and the objects are automatically cleaned up afterwards

See the linked issue above - #2048

@eerhardt eerhardt self-assigned this Aug 2, 2022
@adamsitnik
Copy link
Member

I have few questions regarding:

public partial class File
{
    public static FileInfo CreateTempFile(string? prefix = null, string? suffix = null);
}

I wonder what the typical scenario is going to be: just creating the file or creating and using it right away. If the latter, we could make it more performant by returning FileStream (fewer sys-calls: calling open just once).

Also, is our goal to create a file in temporary directory or a temporary file that disappears after being closed? We could use O_TMPFILE and FILE_ATTRIBUTE_TEMPORARY to implement it.

@Clockwork-Muse
Copy link
Contributor

Also, is our goal to create a file in temporary directory or a temporary file that disappears after being closed?

Part of the problem here is that it depends on the APIs the caller needs to use.
I'm working with Blender via Python at the moment, and am somewhat annoyed because their file API only takes file paths, not file objects, meaning I require closed named files.
It's definitely something we should consider for an overall "tempfile" API, as the earlier link I was pointed to deals with.

@eerhardt
Copy link
Member Author

eerhardt commented Aug 4, 2022

I wonder what the typical scenario is going to be: just creating the file or creating and using it right away. If the latter, we could make it more performant by returning FileStream (fewer sys-calls: calling open just once).

There are many places that call Path.GetTempFileName() today that immediately just turn around and open the file to use it. So I think there is a case to be made about returning a FileStream instead of a FileInfo from CreateTempFile. However, given the many different ways to configure the FileStream, we might want to consider taking a FileStreamOptions. But one issue there is that FileStreamOptions takes a UnixFileMode now, and the underlying mkstemps API doesn't take a mode. So maybe we would need to throw when UnixFileMode was anything other than None.

Given this feedback/questions, and in trying to lock down for .NET 7, I am just going to implement the Directory API above that was approved. We can iterate on the file API in a future version when we have more time to ensure we are getting it right.

Also, is our goal to create a file in temporary directory or a temporary file that disappears after being closed?

I think that might be a good option to add in the future. But I don't think that is the only use case. Looking at how people use the GetTempFileName API, there are places that use the file in multiple processes. Here's one in dotnet add package. Here's another case in MSBuild that wants to conditionally delete the file (or not) based on whether the compilation failed.

eerhardt added a commit to eerhardt/runtime that referenced this issue Aug 4, 2022
CreateTempSubdirectory will create a new uniquely named directory under the temp directory. This allows applications to write temporary files in a co-located directory.

Contributes to dotnet#72881
@GSPP
Copy link

GSPP commented Aug 7, 2022

What is the use case for why an empty file should be created by the File.CreateTempFile API? To me, the most natural usage is to just create a path and then open that path myself. For directories, creation must of course happen.

Name collisions would not be a problem if the name contains a Guid. It is not necessary to take precautions against collisions. A name collision is like winning the lottery 10 times in a row (according to some quick math: 1 to 1 million vs. 1 to 2^128).

@eerhardt
Copy link
Member Author

eerhardt commented Aug 8, 2022

What is the use case for why an empty file should be created by the File.CreateTempFile API?

The same use cases for the existing Path.GetTempFileName() method today.

Name collisions would not be a problem if the name contains a Guid.

The name is not guaranteed to contain a Guid.

@deeprobin
Copy link
Contributor

deeprobin commented Aug 8, 2022

Name collisions would not be a problem if the name contains a Guid.

Also a GUID can collide even if it is only every 340_282_366_920_938_463_463_374_607_431_768_211_455th case.

eerhardt added a commit that referenced this issue Aug 10, 2022
* Add Directory.CreateTempSubdirectory

CreateTempSubdirectory will create a new uniquely named directory under the temp directory. This allows applications to write temporary files in a co-located directory.

Contributes to #72881
@eerhardt eerhardt removed the blocking Marks issues that we want to fast track in order to unblock other important work label Aug 10, 2022
@eerhardt eerhardt modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
@eerhardt eerhardt removed their assignment Sep 30, 2022
@iSazonov
Copy link
Contributor

public static DirectoryInfo CreateTempSubdirectory(string? prefix = null);

Hmm, what is the difference between Directory.CreateDirectory()?

#2048 is exactly what we need.

@eerhardt
Copy link
Member Author

Hmm, what is the difference between Directory.CreateDirectory()?

It's the same difference between mkdir and mkdtemp.

  • Guaranteed to be a new, uniquely named directory
  • On Unix, it gets permissions 700 instead of using the umask.

@iSazonov
Copy link
Contributor

Hmm, what is the difference between Directory.CreateDirectory()?

It's the same difference between mkdir and mkdtemp.

  • Guaranteed to be a new, uniquely named directory
  • On Unix, it gets permissions 700 instead of using the umask.

To create a directory with a unique name and assign a 700 mask, you don't need a new API at all - everything is already there. This has probably been discussed a couple of times before and was the reason for the rejection.

What is not addressed in any way is the actual use cases of temporary entities, where developers have to keep track of garbage removal, while an API can do it automatically.

@KalleOlaviNiemitalo
Copy link

@iSazonov how would you do it then? Directory.CreateDirectory(String, UnixFileMode) lets you specify the 700 mode for a new directory but it doesn't tell you whether a directory at the specified path existed already, so you won't know whether you need to generate a new name and retry.

@iSazonov
Copy link
Contributor

@iSazonov how would you do it then? Directory.CreateDirectory(String, UnixFileMode) lets you specify the 700 mode for a new directory but it doesn't tell you whether a directory at the specified path existed already, so you won't know whether you need to generate a new name and retry.

What do you do now if this API doesn't exist? 😺 I can assume that you are not going to invent something complicated and just use new guid as a name.

@KalleOlaviNiemitalo
Copy link

I used Platform Invoke actually.

@danmoseley
Copy link
Member

is this a candidate for labeling 'help wanted'-- perhaps someone here is interested.

@stephentoub
Copy link
Member

This issue covered three approved APIs:

Path.GetTempFileName();
File.CreateTempFile(string? prefix = null, string? suffix = null);
Directory.CreateTempSubdirectory(string? prefix = null);

The first and third were implemented, and there seem to be a bunch of questions about the usefulness of the second, or whether it's the correct shape. @eerhardt, do we still want to add the second method? Should this issue instead be closed? Or do we want to move it back to api-needs-work for revisions to the second?

@eerhardt
Copy link
Member Author

This issue covered three approved APIs:
The first and third were implemented

Only the third was implemented. The first change was to add [EditorBrowsable(EditorBrowsableState.Never)] to the existing Path.GetTempFIleName() API.

I think we should still consider the second method: File.CreateTempFile. I think moving it back to api-needs-work for revisions to the second method is the right thing to do here.

@eerhardt eerhardt added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Feb 22, 2023
@eerhardt eerhardt removed this from the 8.0.0 milestone Feb 22, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 22, 2023
@Jozkee Jozkee added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Mar 15, 2023
@adamsitnik adamsitnik added this to the 9.0.0 milestone Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.IO needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests