Use GetRandomFileName when creating random temp folder to avoid conflict#5229
Use GetRandomFileName when creating random temp folder to avoid conflict#5229frank-dong-ms-zz merged 5 commits intodotnet:masterfrom
Conversation
mstfbl
left a comment
mstfbl
left a comment
There was a problem hiding this comment.
What is the difference between the default seed of Random() and Environment.TickCount?
I see that RandomUtils.Create() uses Random() as its default seed, and the default seed of Random() is "... is derived from the system clock, which has finite resolution" (source). Could Random() (and thereby RandomUtils.Create() already not be using Environment.TickCount as its default seed?
public static TauswortheHybrid Create()
{
// Seed from a system random.
return new TauswortheHybrid(new Random());
}
|
I think the behavior of default seed of Random is not guaranteed, it depends on net framework or net core, also different version might have different behavior. In reply to: 428586963 [](ancestors = 428586963) |
mstfbl
left a comment
mstfbl
left a comment
There was a problem hiding this comment.
Sounds good, thanks Frank! 
Codecov Report
@@ Coverage Diff @@
## master #5229 +/- ##
=======================================
Coverage 73.47% 73.47%
=======================================
Files 1010 1010
Lines 187988 187974 -14
Branches 20262 20261 -1
=======================================
+ Hits 138118 138120 +2
+ Misses 44385 44373 -12
+ Partials 5485 5481 -4
|
| { | ||
| var rnd = RandomUtils.Create(); | ||
| var rnd = RandomUtils.Create(Guid.NewGuid().GetHashCode()); | ||
| string path; |
There was a problem hiding this comment.
Is there an associated bug? Are we seeing an issue here?
There was a problem hiding this comment.
❔ Why not just use Path.GetRandomFileName()?
There was a problem hiding this comment.
The entire implementation of GetShortTempDir() can be reduced to this:
var path = Path.Combine(Path.GetFullPath(Path.GetTempPath()), "ml_tests", Path.GetRandomFileName());
Directory.CreateDirectory(path);
return path;
The ml_tests subdirectory allows a user to easily identify and delete stray folders created by tests. You definitely don't want to place the folders directly in %TEMP% which is what happens today.
There was a problem hiding this comment.
Yes, please use Path.GetRandomFileName(). The original code had RandomUtils.Create, but we have seen it cause conflicts when it comes to file names. We have started replacing those with Path.GetRandomFileName()
In reply to: 438940202 [](ancestors = 438940202)
There was a problem hiding this comment.
No, I don't have repro, user who reported this issue also don't have repro on his local env, he get error from their product environment but not able to provide a repro
In reply to: 438942892 [](ancestors = 438942892,438936814,438920139)
There was a problem hiding this comment.
I see that Path.GetRandomFileName()is indeed superior at generating file and/or directory names, so I'm in favor of using this too. It does seem to be limited to generating just 11 characters, though this will probably be more than enough.
It also seems interesting that here Path.GetTempPath() has been added, but Path.GetRandomFileName() wasn't. I wonder if there's a reason why Path.GetRandomFileName()` wasn't used in the first place.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, that is great idea, one little modification is I want to change "ml_tests" to "ml_dotnet" as this is not for test usage.
In reply to: 438941863 [](ancestors = 438941863)
harishsk
changed the title
ghost
locked as resolved and limited conversation to collaborators
5 participants
address issue: #5210
We are create random temp folder during model load, sometimes there are file name conflict when load models in multi-threading as the random temp folder name is not seeded.