Use a GUID when creating the temp path #4645

jwood803 · 2020-01-11T16:45:45Z

Addresses #568.

@justinormont Not sure if the placement of it is the best, but it should still prevent the collisions.

Edit: Fixes #568 (added so that issue #568 will close automatically when this PR is successfully merged.)

harishsk · 2020-01-12T00:27:04Z

src/Microsoft.ML.Core/Data/Repository.cs

@@ -256,7 +256,7 @@ protected void GetPath(out string pathEnt, out string pathTemp, string dir, stri
            string root = Path.GetFullPath(DirTemp ?? @"x:\dummy");
            string entityPath = Path.Combine(root, dir ?? "", name);
            entityPath = Path.GetFullPath(entityPath);
-            string tempPath = Path.Combine(root, PathMap.Count.ToString());
+            string tempPath = Path.Combine(root, Guid.NewGuid().ToString());


Can you please help me understand the problem you are trying to fix?

Currently, there's a race condition in creating temporary files, causing a naming collision.

The paths are created as C:\Users\jormont\AppData\Local\Temp\2\TLC_25553D77\0, where:

TLC_ is fixed

25553D77 is a seeded random in hex (bad)

0 is the count [0..N] of folders existing in the parent folder (also bad).

The prefix TLC_ (1) is fine, though should be updated to the current name, perhaps to ML_NET_. Or Microsoft_ML_, though I'd worry about creating too long of paths under Windows ~260 chars, especially as a we move towards a GUID. Note users can have long usernames, plus the names of the files placed in this path.

First issue -- The seeded random number (2) is an issue since running parallelly (or rerunning) with the same seed creates the same "random" folder name. This part is a deterministic collision.

Second issue -- The race condition then occurs when counting how many folders (3) are in the parent folder. When multiple processes/threads check the contents of the folder, they all receive the same number. The purposed fix replaces this count [0..N] with a GUID.

The need is to have unique folders for creating temporary files, otherwise they clobber each other. This is causing a crash and has the possibility of silent failures, especially on macOS/Linux where you can delete and overwrite files with currently open handles.

Currently the way I run into this, is running N parallel copies of the AutoML․NET CLI, which I do for validating changes to the ML․NET AutoML API on ~30 datasets. Some of the processes crash with the error in the linked issue, unless they are started staggered to avoid the race condition.

Pushing deeper into the code, the random number (2) is not seeded, but uses the system seed which is set from the current time. I expect the collision occurs due the remark in System.Random docs, "On most Windows systems, Random objects created within 15 milliseconds of one another are likely to have identical seed values." (src). Hence another race condition in (2).

Hopefully the .NET GUIDs don't have the same 15ms race condition as the RNG.

In the current code, it looks like we are using our own random number generation, which seems odd.
It is okay to use GUIDs. But they seem a bit of overkill.

Path.GetRandomFileName seems to be intended for this kind of scenario.
Would that work better?

@harishsk: You're right, Path.GetRandomFileName() seems more purpose built than a GUID.

Code behind this function -- https://github.com/dotnet/runtime/blob/f57d49bb711fda8ac0bd231229d91919f4fca8e8/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs#L249-L265

@harishsk I had no idea that method existed. Thanks for letting me know! Updated to use it.

src/Microsoft.ML.Core/Data/Repository.cs

codecov · 2020-01-12T11:50:20Z

Codecov Report

Merging #4645 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4645   +/-   ##
=======================================
  Coverage   75.84%   75.84%           
=======================================
  Files         947      947           
  Lines      172184   172184           
  Branches    18579    18579           
=======================================
  Hits       130595   130595           
  Misses      36416    36416           
  Partials     5173     5173

Flag	Coverage Δ
#Debug	`75.84% <0%> (ø)`	⬆️
#production	`71.43% <0%> (ø)`	⬆️
#test	`90.7% <0%> (ø)`	⬆️

justinormont · 2020-01-15T18:46:21Z

src/Microsoft.ML.Core/Data/Repository.cs

@@ -256,7 +256,7 @@ protected void GetPath(out string pathEnt, out string pathTemp, string dir, stri
            string root = Path.GetFullPath(DirTemp ?? @"x:\dummy");
            string entityPath = Path.Combine(root, dir ?? "", name);
            entityPath = Path.GetFullPath(entityPath);
-            string tempPath = Path.Combine(root, PathMap.Count.ToString());
+            string tempPath = Path.Combine(root, Path.GetRandomFileName());
            tempPath = Path.GetFullPath(tempPath);

            string parent = Path.GetDirectoryName(entityPath);


The current function here, GetPath(), creates the path. It's called below in CreateEntry():

machinelearning/src/Microsoft.ML.Core/Data/Repository.cs

Lines 335 to 339 in c29a00b

GetPath(out pathEnt, out pathTemp, dir, name, true);

if (PathMap.ContainsKey(pathEnt))

throw ExceptionContext.ExceptParam(nameof(name), "Duplicate entry: '{0}'", pathEnt);

else

PathMap.Add(pathEnt, pathTemp);

There another minor race condition in how the new path is being checked/added to the dictionary. It's a nit, and minor enough to ignore if you want.

If we want to handle in this PR, we could fix by either adding a lock around the IF/ELSE, or by moving to a ConcurrentDictionary and using its TryAdd() to decide if to throw.

(I tagged this on a line of code to allow for a threaded discussion)

Went the ConcurrentDictionary route, but let me know if that's not the correct implementation of it.

justinormont · 2020-01-16T01:39:22Z

src/Microsoft.ML.Core/Data/Repository.cs

@@ -333,10 +334,8 @@ public Entry CreateEntry(string dir, string name)
            string pathEnt;
            string pathTemp;
            GetPath(out pathEnt, out pathTemp, dir, name, true);
-            if (PathMap.ContainsKey(pathEnt))
+            if (PathMap.TryAdd(pathEnt, pathTemp))


Assuming I'm reading the docs right, TryAdd() returns false when it doesn't add (docs).

Hence need to invert the return value.

Suggested change

if (PathMap.TryAdd(pathEnt, pathTemp))

if (!PathMap.TryAdd(pathEnt, pathTemp))

Whoops! I thought I added that, but I must have missed it. Thanks for catching!

justinormont · 2020-01-16T01:42:11Z

src/Microsoft.ML.Core/Data/Repository.cs

@@ -525,7 +524,7 @@ public Entry OpenEntryOrNull(string dir, string name)
                    // Extract to a temporary file.
                    Directory.CreateDirectory(Path.GetDirectoryName(pathTemp));
                    entry.ExtractToFile(pathTemp);
-                    PathMap.Add(pathLower, pathTemp);
+                    PathMap.TryAdd(pathLower, pathTemp);


To maintain similar to the previous behavior, we'll have to throw if the TryAdd() returns false.

The previous Dictionary.Add() throws an ArgumentException if the value being added already exists (docs).

Suggested change

PathMap.TryAdd(pathLower, pathTemp);

if (!PathMap.TryAdd(pathLower, pathTemp))

throw ExceptionContext.ExceptParam(nameof(name), "Duplicate entry: '{0}'", pathLower);

Updated to throw exception.

justinormont · 2020-01-16T18:06:29Z

I restarted the failed tests, hopefully they succeed.

mstfbl · 2020-01-27T10:46:52Z

Restarting failed tests one more time. There is currently an issue with the CI "Windows_x64_NetCoreApp21 Release_Build".

justinormont · 2020-01-29T20:07:37Z

@jwood803: Can you pull in changes from the master branch? This should allow the CI tests to pass (various flaky unit tests were temporarily disabled).

Use a GUID when creating the temp path

52f321d

jwood803 requested a review from a team as a code owner January 11, 2020 16:45

Update where the GUID is in the temp path

280e9ad

harishsk reviewed Jan 12, 2020

View reviewed changes

src/Microsoft.ML.Core/Data/Repository.cs Outdated Show resolved Hide resolved

Remove braces

fc636e4

Use random file name instead of GUID

c29a00b

harishsk approved these changes Jan 15, 2020

View reviewed changes

justinormont reviewed Jan 15, 2020

View reviewed changes

justinormont approved these changes Jan 15, 2020

View reviewed changes

Switch to concurrent dictionary

0f06dfb

justinormont reviewed Jan 16, 2020

View reviewed changes

TryAdd updates

ca78b2c

mstfbl mentioned this pull request Jan 27, 2020

Temporary filename collision #568

Closed

Merge branch 'master' into temp-files

34c7bb2

justinormont merged commit 1ef5fa9 into dotnet:master Jan 31, 2020

This was referenced Jan 31, 2020

Default seed is not propagated from MLContext #4752

Closed

Data splits to default to MLContext seed when not specified #4764

Merged

This was referenced Jun 11, 2020

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

Merged

File already exists error when loading LightGbmBinaryTrainer from MemoryStream #5210

Closed

ghost locked as resolved and limited conversation to collaborators Mar 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a GUID when creating the temp path #4645

Use a GUID when creating the temp path #4645

jwood803 commented Jan 11, 2020 •

edited by mstfbl

Loading

harishsk Jan 12, 2020

justinormont Jan 12, 2020 •

edited

Loading

justinormont Jan 12, 2020

harishsk Jan 14, 2020

justinormont Jan 15, 2020

jwood803 Jan 15, 2020

codecov bot commented Jan 12, 2020 •

edited

Loading

justinormont Jan 15, 2020

jwood803 Jan 15, 2020

justinormont Jan 16, 2020

jwood803 Jan 16, 2020

justinormont Jan 16, 2020

jwood803 Jan 16, 2020

justinormont commented Jan 16, 2020

mstfbl commented Jan 27, 2020 •

edited

Loading

justinormont commented Jan 29, 2020

	GetPath(out pathEnt, out pathTemp, dir, name, true);
	if (PathMap.ContainsKey(pathEnt))
	throw ExceptionContext.ExceptParam(nameof(name), "Duplicate entry: '{0}'", pathEnt);
	else
	PathMap.Add(pathEnt, pathTemp);

	if (PathMap.TryAdd(pathEnt, pathTemp))
	if (!PathMap.TryAdd(pathEnt, pathTemp))

	PathMap.TryAdd(pathLower, pathTemp);
	if (!PathMap.TryAdd(pathLower, pathTemp))
	throw ExceptionContext.ExceptParam(nameof(name), "Duplicate entry: '{0}'", pathLower);

Use a GUID when creating the temp path #4645

Use a GUID when creating the temp path #4645

Conversation

jwood803 commented Jan 11, 2020 • edited by mstfbl Loading

Choose a reason for hiding this comment

justinormont Jan 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinormont commented Jan 16, 2020

mstfbl commented Jan 27, 2020 • edited Loading

justinormont commented Jan 29, 2020

jwood803 commented Jan 11, 2020 •

edited by mstfbl

Loading

justinormont Jan 12, 2020 •

edited

Loading

codecov bot commented Jan 12, 2020 •

edited

Loading

mstfbl commented Jan 27, 2020 •

edited

Loading