Add Directory.CreateTempSubdirectory by eerhardt · Pull Request #73408 · dotnet/runtime

eerhardt · 2022-08-04T22:10:53Z

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

Note: I've decided not to implement the full approved API in #72881 because of the feedback on the File API. That will be added in a future PR.

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

ghost · 2022-08-04T22:11:00Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-08-04T22:11:21Z

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

Issue Details

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

Note: I've decided not to implement the full approved API in #72881 because of the feedback on the File API. That will be added in a future PR.

Author:	eerhardt
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`
Milestone:	-

stephentoub · 2022-08-05T00:53:06Z

+ while (attempts < MaxAttempts)
+ {
+ Interop.GetRandomBytes(pKey, RandomKeyLength);
+ Path.Populate83FileNameFromRandomBytes(pKey, RandomKeyLength, builder.RawChars.Slice(builder.Length, RandomFileNameLength));


Why is it important to use the 8.3 naming?

It isn't important. But it is what GetRandomFileName uses
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs
Lines 281 to 289 in 916f1ad
publicstaticunsafestringGetRandomFileName()
{
byte*pKey=stackallocbyte[KeyLength];
Interop.GetRandomBytes(pKey,KeyLength);
returnstring.Create(
12,(IntPtr)pKey,(span,key)=>// 12 == 8 + 1 (for period) + 3
Populate83FileNameFromRandomBytes((byte*)key,KeyLength,span));
}
I'm just simulating a call to GetRandomFileName here, without allocating an intermediate string.

I can add a comment here, if that helps. Or did you have a different algorithm in mind for generating a folder name?

IMO having a period in the directory name seems a bit strange

IMO having a period in the directory name seems a bit strange
Does it really matter though? Code shouldn't depend on these names - they even have a different pattern between Windows and Unix. Besides, there are plenty of cases where directories have periods in their names - versions, namespace-like, etc.
e.g. C:\Program Files\dotnet\shared\Microsoft.NETCore.App\6.0.7.

jozkee

Other than existing comments and a few more I'm leaving, LGTM.

- Rewrite CreateTempSubdirectory and GetTempFileName on Unix to minimize allocations.

eerhardt · 2022-08-05T22:09:22Z

I have responded to all existing feedback and pushed new changes. PTAL.

tmds · 2022-08-07T12:20:24Z

+
+namespace System.IO.Tests
+{
+ public class Directory_CreateTempSubdirectory : FileSystemTest


Maybe verify that the filemode of the created directory is 700 on Unix. That is an important trait of this API.

Q: is it on Windows also guaranteed the resulting directory is only accessible to the current user?

Maybe verify that the filemode of the created directory is 700 on Unix.
Done.
Q: is it on Windows also guaranteed the resulting directory is only accessible to the current user?
No, on Windows there is no guarantee on the permissions of the directory. Just that it is empty, and it is in the TEMP directory.

adamsitnik

LGTM, thanks a lot @eerhardt !

adamsitnik

…tests These tests had 3 methods that do the same thing. Consolidating down to one. Follow up to dotnet#73408

Add Directory.CreateTempSubdirectory
9f3ffa3
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

eerhardt requested review from adamsitnik, jozkee and stephentoub August 4, 2022 22:10

ghost added area-System.IO new-api-needs-documentation labels Aug 4, 2022

ghost assigned eerhardt Aug 4, 2022

jkotas reviewed Aug 4, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.cs Outdated

stephentoub reviewed Aug 5, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.Unix.cs Outdated

stephentoub reviewed Aug 5, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.Unix.cs Outdated

stephentoub reviewed Aug 5, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.Unix.cs Outdated

stephentoub reviewed Aug 5, 2022
View reviewed changes

This was referenced Aug 5, 2022
Infra improvements for Helix #68176
Closed
GC/API/GC/GetGCMemoryInfo/GetGCMemoryInfo.sh test failing intermittently on CoreCLR Linux ARM32 #73247
Closed

jozkee approved these changes Aug 5, 2022
View reviewed changes

PR feedback
692c536

danmoseley reviewed Aug 5, 2022
View reviewed changes

Comment threadsrc/libraries/System.IO.FileSystem/tests/Directory/CreateTempSubdirectory.cs Outdated

eerhardt added 2 commits August 5, 2022 16:47

PR feedback
5b86fd3
- Rewrite CreateTempSubdirectory and GetTempFileName on Unix to minimize allocations.

Fix tests on Windows.
d744ac9

Nirmal4G reviewed Aug 6, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.cs

tmds reviewed Aug 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Runtime/ref/System.Runtime.cs

Assert UnixFileMode is 700 for created temp directory.
dc910fd

adamsitnik approved these changes Aug 9, 2022
View reviewed changes

eerhardt added 2 commits August 9, 2022 15:01

PR feedback
5fd8d41

Rename ThrowExceptionForLastError to ThrowIOExceptionForLastError
e9f0a01

adamsitnik approved these changes Aug 10, 2022
View reviewed changes

eerhardt merged commit 9fd407a into dotnet:mainAug 10, 2022

eerhardt deleted the AddCreateTempDirectory branch August 10, 2022 12:43

radical mentioned this pull request Aug 10, 2022
Path.GetTempFileName() sometimes fails on WASM due to insufficient randomness #73721
Closed

eerhardt added a commit to eerhardt/runtime that referenced this pull request Aug 12, 2022
Clean up temp directory methods in System.ComponentModel.Composition …
e247628
…tests These tests had 3 methods that do the same thing. Consolidating down to one. Follow up to dotnet#73408

eerhardt mentioned this pull request Aug 12, 2022
Clean up temp directory methods in System.ComponentModel.Composition tests #73857
Merged

danmoseley mentioned this pull request Aug 15, 2022
Unix: Move 'FileNotFound to DirectoryNotFound' handling to GetExceptionForIoErrno. #70661
Merged

ghost locked as resolved and limited conversation to collaborators Sep 9, 2022

	publicstaticunsafestringGetRandomFileName()
	{
	byte*pKey=stackallocbyte[KeyLength];
	Interop.GetRandomBytes(pKey,KeyLength);

	returnstring.Create(
	12,(IntPtr)pKey,(span,key)=>// 12 == 8 + 1 (for period) + 3
	Populate83FileNameFromRandomBytes((byte*)key,KeyLength,span));
	}

Uh oh!

Conversation

eerhardt commented Aug 4, 2022

Uh oh!

ghost commented Aug 4, 2022

Uh oh!

ghost commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoubAug 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardtAug 5, 2022

Choose a reason for hiding this comment

Uh oh!

eerhardtAug 5, 2022

Choose a reason for hiding this comment

Uh oh!

saulAug 9, 2022

Choose a reason for hiding this comment

Uh oh!

eerhardtAug 9, 2022

Choose a reason for hiding this comment

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Aug 5, 2022

Uh oh!

Uh oh!

tmdsAug 7, 2022

Choose a reason for hiding this comment

Uh oh!

tmdsAug 7, 2022

Choose a reason for hiding this comment

Uh oh!

eerhardtAug 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

stephentoubAug 5, 2022•
edited
Loading