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

Fix GetTempFileName() on Windows #74855

Merged
merged 14 commits into from Sep 2, 2022
Merged

Fix GetTempFileName() on Windows #74855

merged 14 commits into from Sep 2, 2022

Conversation

danmoseley
Copy link
Member

Fix #73793

  1. Change from GetTempFileNameW to generating a random file with the same pattern we use on Unix - tmpXXXXXX.tmp.
  2. Tweak an error string to make it more specific.
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.1889 (21H2)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-rc.2.22419.24
  [Host]     : .NET 7.0.0 (7.0.22.41112), X64 RyuJIT
  Job-SVOUAO : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT
  Job-YORQJO : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT

InvocationCount=1  UnrollFactor=1

|          Method |        Job |         Toolchain |     Mean |    Error |   StdDev |   Median | Ratio | RatioSD | Allocated |
|---------------- |----------- |------------------ |---------:|---------:|---------:|---------:|------:|--------:|----------:|
| GetTempFileName | Job-SVOUAO | \main\corerun.exe | 353.0 us | 21.81 us | 63.28 us | 343.2 us |  1.00 |    0.00 |         - |
| GetTempFileName | Job-YORQJO |   \pr\corerun.exe | 300.2 us | 17.85 us | 50.92 us | 279.3 us |  0.86 |    0.19 |         - |

That's with the best situation, where there's no existing temp files. When the temp folder is nearly full, the existing API deteriorates substantially:

|          Method |        Job |         Toolchain |        Mean |        Error |        StdDev |      Median | Ratio | RatioSD | Allocated |
|---------------- |----------- |------------------ |------------:|-------------:|--------------:|------------:|------:|--------:|----------:|
| GetTempFileName | Job-IBETAP | \main\corerun.exe | 94,514.5 us | 59,324.54 us | 160,387.33 us | 13,492.7 us |  1.00 |    0.00 |         - |
| GetTempFileName | Job-NDLNYW |   \pr\corerun.exe |    310.0 us |     16.70 us |      48.18 us |    297.4 us |  0.09 |    0.16 |         - |

This API will be hidden as part of #72881, but a great deal of code has been written to use it and that code will become faster/more robust. Incidentally, in testing this I filled up the temp folder and (with the previous implementation) Benchmark .NET crashed. That will now be fixed.

@ghost
Copy link

ghost commented Aug 31, 2022

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

Issue Details

Fix #73793

  1. Change from GetTempFileNameW to generating a random file with the same pattern we use on Unix - tmpXXXXXX.tmp.
  2. Tweak an error string to make it more specific.
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.1889 (21H2)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-rc.2.22419.24
  [Host]     : .NET 7.0.0 (7.0.22.41112), X64 RyuJIT
  Job-SVOUAO : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT
  Job-YORQJO : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT

InvocationCount=1  UnrollFactor=1

|          Method |        Job |         Toolchain |     Mean |    Error |   StdDev |   Median | Ratio | RatioSD | Allocated |
|---------------- |----------- |------------------ |---------:|---------:|---------:|---------:|------:|--------:|----------:|
| GetTempFileName | Job-SVOUAO | \main\corerun.exe | 353.0 us | 21.81 us | 63.28 us | 343.2 us |  1.00 |    0.00 |         - |
| GetTempFileName | Job-YORQJO |   \pr\corerun.exe | 300.2 us | 17.85 us | 50.92 us | 279.3 us |  0.86 |    0.19 |         - |

That's with the best situation, where there's no existing temp files. When the temp folder is nearly full, the existing API deteriorates substantially:

|          Method |        Job |         Toolchain |        Mean |        Error |        StdDev |      Median | Ratio | RatioSD | Allocated |
|---------------- |----------- |------------------ |------------:|-------------:|--------------:|------------:|------:|--------:|----------:|
| GetTempFileName | Job-IBETAP | \main\corerun.exe | 94,514.5 us | 59,324.54 us | 160,387.33 us | 13,492.7 us |  1.00 |    0.00 |         - |
| GetTempFileName | Job-NDLNYW |   \pr\corerun.exe |    310.0 us |     16.70 us |      48.18 us |    297.4 us |  0.09 |    0.16 |         - |

This API will be hidden as part of #72881, but a great deal of code has been written to use it and that code will become faster/more robust. Incidentally, in testing this I filled up the temp folder and (with the previous implementation) Benchmark .NET crashed. That will now be fixed.

Author: danmoseley
Assignees: danmoseley
Labels:

area-System.IO

Milestone: -

@danmoseley
Copy link
Member Author

Updated.

|          Method |        Job |         Toolchain |     Mean |   Error |  StdDev | Ratio | RatioSD | Allocated |
|---------------- |----------- |------------------ |---------:|--------:|--------:|------:|--------:|----------:|
| GetTempFileName | Job-FYMLRF | \main\corerun.exe | 362.1 us | 3.76 us | 3.33 us |  1.00 |    0.00 |     112 B |
| GetTempFileName | Job-VTFTPP |   \pr\corerun.exe | 341.6 us | 6.79 us | 9.06 us |  0.94 |    0.02 |     392 B |

for convenience, the above includes the cleanup: public string GetTempFileName() { string path = Path.GetTempFileName(); File.Delete(path); return path; }.

@danmoseley
Copy link
Member Author

I think this is good to go. It does feel like this and GetTempFileName ideally would use 36 not 32 chars, giving another 4000 possibilities. I can change both if folks want. But 1 billion is a lot better than 65K.

@danmoseley
Copy link
Member Author

Addressed feedback again.

@danmoseley danmoseley closed this Sep 1, 2022
@danmoseley danmoseley reopened this Sep 1, 2022
@danmoseley danmoseley closed this Sep 1, 2022
@danmoseley danmoseley reopened this Sep 1, 2022
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is great. Now we can recommend using this API without having to add the caveat of "on Windows this is limited to 65K files".

We should also be sure to update the docs to remove that part for .NET 8:

https://docs.microsoft.com/en-us/dotnet/api/system.io.path.gettempfilename

The GetTempFileName method will raise an IOException if it is used to create more than 65535 files without deleting previous temporary files.

@danmoseley
Copy link
Member Author

I think everything's addressed, unless you want me to remove the ToLowerInvariant.

@stephentoub
Copy link
Member

I think everything's addressed, unless you want me to remove the ToLowerInvariant.

I mean, only if you care about saving the trees.

@danmoseley
Copy link
Member Author

@stephentoub I noticed that GetTempFileName() on Unix throws FileNotFoundException when TMPDIR is bad. Windows throws DirectoryNotFoundException when TMP is bad. I fixed Unix - LMK if you have concerns. Perhaps a breaking change note to make.

where TMPDIR was /tmp/tmp7xcc3h.tmp/ it was Could not find file '/tmp/tmp7xcc3h.tmp/'. and it is now Could not find a part of the path '/tmp/tmp7xcc3h.tmp/'. with the correct DirectoryNotFoundException type.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @danmoseley !

@adamsitnik adamsitnik added this to the 8.0.0 milestone Sep 2, 2022
@danmoseley
Copy link
Member Author

Failures are QUIC and LibrariesInteropGenerator

@danmoseley danmoseley merged commit 174e043 into dotnet:main Sep 2, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Oct 2, 2022
@danmoseley danmoseley deleted the gtp2 branch December 15, 2022 02:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more entropy to GetTempFileName() on Windows
6 participants