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 a GUID when creating the temp path #4645

Merged
merged 7 commits into from Jan 31, 2020
Merged

Use a GUID when creating the temp path #4645

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
52f321d
Use a GUID when creating the temp path
jwood803 Jan 11, 2020
280e9ad
Update where the GUID is in the temp path
jwood803 Jan 11, 2020
fc636e4
Remove braces
jwood803 Jan 12, 2020
c29a00b
Use random file name instead of GUID
jwood803 Jan 15, 2020
0f06dfb
Switch to concurrent dictionary
jwood803 Jan 15, 2020
ca78b2c
TryAdd updates
jwood803 Jan 16, 2020
34c7bb2
Merge branch 'master' into temp-files
jwood803 Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/Microsoft.ML.Core/Data/Repository.cs
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -93,7 +94,7 @@ public void Dispose()
// more than once.
// REVIEW: Should we garbage collect to some degree? Currently we don't delete any
// of these temp files until the repository is disposed.
protected readonly Dictionary<string, string> PathMap;
protected readonly ConcurrentDictionary<string, string> PathMap;

/// <summary>
/// Exception context.
Expand All @@ -107,7 +108,7 @@ internal Repository(bool needDir, IExceptionContext ectx)
Contracts.AssertValueOrNull(ectx);
_ectx = ectx;

PathMap = new Dictionary<string, string>();
PathMap = new ConcurrentDictionary<string, string>();
_open = new List<Entry>();
if (needDir)
DirTemp = GetShortTempDir();
Expand Down Expand Up @@ -256,7 +257,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Stream stream;
if (pathTemp != null)
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to throw exception.

stream = new FileStream(pathTemp, FileMode.Open, FileAccess.Read);
}
else
Expand Down