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.

Sign up for GitHub

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

Jump to bottom

Use GetRandomFileName when creating random temp folder to avoid conflict #5229

Merged
merged 5 commits into from
Jun 11, 2020
Merged

Use GetRandomFileName when creating random temp folder to avoid conflict #5229

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c0a70c
add random seed when create random temp folder to avoid conflict
frank-dong-ms Jun 11, 2020
e1a7272
use guid hash instead of system time to generate random number
frank-dong-ms Jun 11, 2020
5177ac6
take comments
frank-dong-ms Jun 11, 2020
dc9b828
code clean up
frank-dong-ms Jun 11, 2020
8f58ec8
fix bug
frank-dong-ms Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Core/Data/Repository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ internal Repository(bool needDir, IExceptionContext ectx)

private static string GetShortTempDir()
{
var rnd = RandomUtils.Create();
var rnd = RandomUtils.Create(Guid.NewGuid().GetHashCode());
string path;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an associated bug? Are we seeing an issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this PR is to fix below issue:
#5210

In reply to: 438920139 [](ancestors = 438920139)

Copy link
Member

Choose a reason for hiding this comment

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

❔ Why not just use Path.GetRandomFileName()?

Copy link
Member

@sharwell sharwell Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Thanks @sharwell and @harishsk for the heads up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that make sense

In reply to: 438942324 [](ancestors = 438942324,438940202)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@frank-dong-ms-zz frank-dong-ms-zz Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

checked the implementation again, Path.GetRandomFileName() is already added at line 239 by Jon Wood at 1/31/2020 at below PR:
#4645

so consider this PR as code cleanup

In reply to: 438971881 [](ancestors = 438971881,438941863)

do
{
Expand Down