Fixed issue #970 #971
Fixed issue #970 #971
Conversation
|
LGTM. I was wondering is XUnit supports any kind of debug output so we can tell the tester at the end: yo, something screwed up, you have to manually delete the files (residual files can cause subsequent failures if same hash ends up used). |
| { | ||
| File.Delete(file); | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
I'd rather us to be a little more specific here. Could you please amend your commit (ie. not fixing this through another additional commit) and make this rather catch UnauthorizedAccessException and this is what #970 is about?
|
While you're tweaking your commit, could you please reword your commit message so that the reader can understand what the commit does by reading through You can peek at caded6e for a recent great commit message. You can also see https://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message for some good background details. |
|
@Therzok I'm pretty new to xUnit (only used NUnit and JUnit before), so I can't answer your question. But it would certainly be a good idea to either provide a message or attempt a cleanup to at least limit the number of leftover files or directories. @nulltoken I caught an |
|
@ThomasBarnekow Let's walk this path step by step. We know that |
2429805 to
b6fdb68
Compare
| catch (UnauthorizedAccessException ex) | ||
| { | ||
| Trace.WriteLine(GetErrorMessage("file", file, ex)); | ||
| } |
There was a problem hiding this comment.
@nulltoken In this amended commit, both exceptions are caught separately and will produce an error message that differentiates between the two types of commits (at least by saying what type of exception was thrown).
|
@nulltoken You might still want to have a look at those messages as this might provide some insight as to what other issue might be behind this. Here's an interesting example because both On an unrelated note, there might be some potential for streamlining the output. When an exception is thrown for a file or directory, the same message is repeated for all ancestors. |
|
@ThomasBarnekow Neat! It may worth it to actually also render the exception message. Something is keeping a hold on those objects. How do you run your tests? Through an IDE (which one, which version, which test runner)? Through an external UI? Can you repro this behavior by running the |
|
@nulltoken I've just run another test to compare the output with the previous test's output. Now, I've seen this exception (I believe for the first time): The following statement produces that exception: It looks like any file system operation can potentially throw an exception (IOException or UnauthorizedAccessException). Therefore, I'm thinking that we should probably fix this a little differently. I'd propose to catch the exceptions in the This assumes the
I'll wait with further commits until I've seen your thoughts on this. Maybe @Therzok would like to chime in as well. |
|
Something is keeping a hold on those objects. How do you run your tests? Through an IDE (which one, which version, which test runner)? Through an external UI? Can you reproduce this behavior by running the Can you try whitelisting the LibGit2Sharp directory path in you anti-virus? |
|
@nulltoken I'm using Visual Studio 2010 and the xUnit GUI test runner which I've added as an external tool so I can select the LibGit2Sharp.Tests project and then run tests by clicking Tools - xUnit GUI Test. Here's the configuration: And yes, we should render the exception message, in particular if we decide to only catch exceptions in the Let me try running the |
|
@nulltoken just ran Will see whether I can whitelist the a directory in my anti-virus. |
|
@nulltoken While I'm not sure whether that covered all the security tools installed on my machine, I've at least configured an exception in my Symantec AntiVirus. Running What might be causing this, however, could be a totally different thing. I'm using TortoiseGit, which also scans Git repositories to produce icon overlays. This might get in the way of removing these folders. |
|
Just uninstalled TortoiseGit (because there is no way to deactivate the scan) and temporarily deactivated all malware and virus scanners I see on my machine. Nevertheless, the same exceptions are thrown (but caught). |
|
Thanks for investigated this further! I cannot reproduce this through xUnit Gui runner. Hrmpf... 😕 Let's move forward. Could you please amend your commit by adding the exception message so we can get this in? Once this is done, could you also please rebase your branch onto the latest vNext so that we can fast forward merge it? |
|
I made some further changes to the For the rare case that an I'll amend and rebase as you said. |
|
I'm now pretty sure that TortoiseGit was causing most of my problems:
Fortunately, TortoiseGit can also be configured to exclude the bin folder of LibGit2Sharp.Tests. I've therefore added that to the list of known issues in the error message. |
|
Unfortunately, I'm not a TortoiseGit user. @csware Any idea? |
b6fdb68 to
d007b4f
Compare
|
@nulltoken TortoiseGit uses a background process called TGitCache to analyze the status of git repositories and then show corresponding icon overlays in Explorer. This interferes with the As I said, this can be configured so it no longer interferes (although the |
| catch (UnauthorizedAccessException ex) | ||
| { | ||
| Trace.WriteLine(GetErrorMessage(directory, ex)); | ||
| } |
There was a problem hiding this comment.
The UnauthorizedAccessException must be caught because it is still thrown by DirectoryHelper.DeleteDirectory() if it is caught a second time.
|
@ThomasBarnekow Which version of TortoiseGit? |
| "{0}- Antivirus (exclude the bin folder of LibGit2Sharp.Tests from the paths scanned by your real-time antivirus)" + | ||
| "{0}- TortoiseGit (change the 'Icon Overlays' settings, e.g., adding the bin folder of LibGit2Sharp.Tests to 'Exclude paths' and appending an '*' to exclude all subfolders as well)", | ||
| Environment.NewLine, Path.GetFullPath(path), ex.GetType(), ex.Message); | ||
| } |
There was a problem hiding this comment.
I've moved this method over from the DirectoryHelper class because it is no longer required there. I've also added TortoiseGit as a known cause for the error.
|
@csware I'm using TortoiseGit 1.8.13.0. |
| @@ -71,16 +72,37 @@ public static void DeleteDirectory(string directoryPath) | |||
| File.SetAttributes(directoryPath, FileAttributes.Normal); | |||
| try | |||
| { | |||
| Directory.Delete(directoryPath, false); | |||
| Directory.Delete(directoryPath, true); | |||
There was a problem hiding this comment.
This also looks unrelated. Please revert this change.
There was a problem hiding this comment.
This is not unrelated because files are no longer deleted individually. Reverting this change would break the fix.
There was a problem hiding this comment.
Oh! I missed the removal of File.Delete() above.
What kind of exception would we write out to the trace would one file be held by another process while recursively removing a directory up the hierarchy? Would it clearly specify the name of the file?
There was a problem hiding this comment.
That would be the anUnauthorizedAccessException with the exact file name. This is done in the BaseFixture.Dispose() method.
This is also the only place were we really need to write out anything to the trace. If DirectoryHelper.DeleteDirectory() doesn't throw, it successfully removed the directory, possibly retrying once or twice.
d007b4f to
dfbb8d3
Compare
|
@nulltoken All logging is now turned on. |
|
@ThomasBarnekow You'll find below a counter proposal diff --git a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
index fdb8135..30c0631 100644
--- a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
+++ b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
@@ -209,14 +209,7 @@ public virtual void Dispose()
{
foreach (string directory in directories)
{
- try
- {
- DirectoryHelper.DeleteDirectory(directory);
- }
- catch (UnauthorizedAccessException ex)
- {
- Trace.WriteLine(GetErrorMessage(directory, ex));
- }
+ DirectoryHelper.DeleteDirectory(directory);
}
#if LEAKS_IDENTIFYING
@@ -232,17 +225,6 @@ public virtual void Dispose()
#endif
}
- private static string GetErrorMessage(string path, Exception ex)
- {
- return string.Format("{0}The directory '{1}' could not be deleted due to a {2}: {3}" +
- "{0}Most of the time, this is due to an external process accessing the files in the temporary repositories created during the test runs, and keeping a handle on the directory, thus preventing the deletion of those files." +
- "{0}Known and common causes include:" +
- "{0}- Windows Search Indexer (go to the Indexing Options, in the Windows Control Panel, and exclude the bin folder of LibGit2Sharp.Tests)" +
- "{0}- Antivirus (exclude the bin folder of LibGit2Sharp.Tests from the paths scanned by your real-time antivirus)" +
- "{0}- TortoiseGit (change the 'Icon Overlays' settings, e.g., adding the bin folder of LibGit2Sharp.Tests to 'Exclude paths' and appending an '*' to exclude all subfolders as well)",
- Environment.NewLine, Path.GetFullPath(path), ex.GetType(), ex.Message);
- }
-
protected static void InconclusiveIf(Func<bool> predicate, string message)
{
if (!predicate())
diff --git a/LibGit2Sharp.Tests/TestHelpers/DirectoryHelper.cs b/LibGit2Sharp.Tests/TestHelpers/DirectoryHelper.cs
index 25a7d85..094096e 100644
--- a/LibGit2Sharp.Tests/TestHelpers/DirectoryHelper.cs
+++ b/LibGit2Sharp.Tests/TestHelpers/DirectoryHelper.cs
@@ -2,14 +2,13 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
+using System.Linq;
+using System.Threading;
namespace LibGit2Sharp.Tests.TestHelpers
{
public static class DirectoryHelper
{
- // Set this to true to have DirectoryHelper.Delete() write verbose trace messages.
- private static bool verbose = true;
-
private static readonly Dictionary<string, string> toRename = new Dictionary<string, string>
{
{ "dot_git", ".git" },
@@ -50,58 +49,71 @@ public static void DeleteDirectory(string directoryPath)
if (!Directory.Exists(directoryPath))
{
- Trace.WriteLineIf(verbose, string.Format("Directory '{0}' is missing and can't be removed.", directoryPath));
+ Trace.WriteLine(string.Format("Directory '{0}' is missing and can't be removed.", directoryPath));
return;
}
+ DeleteDirectory_Internal(directoryPath);
+ }
+
+ private static void DeleteDirectory_Internal(string directoryPath)
+ {
+ NormalizeAttributes(directoryPath);
+
+ TryRecurseDelete(directoryPath, 5, 16, 2);
+ }
+
+ private static void NormalizeAttributes(string directoryPath)
+ {
string[] files = Directory.GetFiles(directoryPath);
string[] dirs = Directory.GetDirectories(directoryPath);
foreach (string file in files)
{
- // We only set the file's attributes at this point and later delete its parent
- // directory recursively, at which time we will also deal with exceptions.
File.SetAttributes(file, FileAttributes.Normal);
}
foreach (string dir in dirs)
{
- DeleteDirectory(dir);
+ NormalizeAttributes(dir);
}
File.SetAttributes(directoryPath, FileAttributes.Normal);
- try
- {
- Directory.Delete(directoryPath, true);
- }
- catch (IOException ex1)
- {
- // Retry if the directory was allegedly not found or not empty.
- Trace.WriteLineIf(verbose, "Retrying to delete '" + directoryPath + "' after catching " + ex1.GetType());
- Directory.Delete(directoryPath, true);
- }
- catch (UnauthorizedAccessException ex1)
+ }
+
+ private static void TryRecurseDelete(
+ string directoryPath,
+ int numberOfAttempts, int initialThreadSleepDuration, int durationMultiplier)
+ {
+ var whitelist = new[] { typeof (IOException), typeof (UnauthorizedAccessException) };
+
+ for (var i=1; i <= numberOfAttempts; i++)
{
try
{
- // Retry if there was an access problem. But wait a little while before trying again,
- // because the issue might no longer exist then. Sleep(0) did not help, so using 100.
- System.Threading.Thread.Sleep(100);
-
- Trace.WriteLineIf(verbose, "Retrying to delete '" + directoryPath + "' after catching " + ex1.GetType());
Directory.Delete(directoryPath, true);
}
- catch (IOException ex2)
+ catch (Exception e)
{
- // Retry if the directory was allegedly not found or not empty.
- Trace.WriteLineIf(verbose, "Re trying to delete '" + directoryPath + "' after catching " + ex2.GetType());
- Directory.Delete(directoryPath, true);
- }
- catch (UnauthorizedAccessException ex2)
- {
- // Don't attempt another deletion if this exception is caught a second time.
- Trace.WriteLineIf(verbose, "Not trying to delete '" + directoryPath + "' again after catching " + ex2.GetType() + " a second time");
- throw;
+ if (!whitelist.Contains(e.GetType()))
+ {
+ throw;
+ }
+
+ if (i != numberOfAttempts)
+ {
+ var sleepDuration = initialThreadSleepDuration * Math.Pow(durationMultiplier, (i -1));
+ Thread.Sleep((int)sleepDuration);
+ continue;
+ }
+
+ Trace.WriteLine(string.Format("{0}The directory '{1}' could not be deleted ({2} attempts were made) due to a {3}: {4}" +
+ "{0}Most of the time, this is due to an external process accessing the files in the temporary repositories created during the test runs, and keeping a handle on the directory, thus preventing the deletion of those files." +
+ "{0}Known and common causes include:" +
+ "{0}- Windows Search Indexer (go to the Indexing Options, in the Windows Control Panel, and exclude the bin folder of LibGit2Sharp.Tests)" +
+ "{0}- Antivirus (exclude the bin folder of LibGit2Sharp.Tests from the paths scanned by your real-time antivirus)" +
+ "{0}- TortoiseGit (change the 'Icon Overlays' settings, e.g., adding the bin folder of LibGit2Sharp.Tests to 'Exclude paths' and appending an '*' to exclude all subfolders as well)",
+ Environment.NewLine, Path.GetFullPath(directoryPath), numberOfAttempts, e.GetType(), e.Message));
}
}
}Thoughts? |
|
@nulltoken That is certainly a more flexible approach. Did you test it with TortoiseGit's TGitCache background process trying to do its thing on the test repos? Did it remove all directories? I can change my stuff to reflect your proposal. But I think this could be done a bit more elegantly, like so: This is not tested, it just compiles. |
Recursing from within a
Unfortunately, I don't use TortoiseGit. I've done some tests keeping a hold on some files/folders to ensure that the code was behaving as expected, but I'd very grateful if you could check this against your own environment. |
dfbb8d3 to
33b8f14
Compare
|
@nulltoken Have a look. This is essentially what you proposed with two fixes and minor modifications. |
The DirectoryHelper.DeleteDirectory() method throws exceptions. Affected tests will be reported as FAILED although this happens during cleanup after the test was run. This commit deals with those exceptions by retrying to delete the directory up to four times, which removes directories in all but rare cases. The method catches DirectoryNotFoundException, IOException, and UnauthorizedAccessException and will only log a trace message if it can't finally delete the directory. It will rethrow any other type of exception. Closes libgit2#970.
33b8f14 to
3afb175
Compare
| try | ||
| { | ||
| Directory.Delete(directoryPath, true); | ||
| return; |
There was a problem hiding this comment.
That return statement was missing in your proposal. However, we don't want to continue after a successful attempt.
|
👍 Cheers! |
This pull request fixes issue #970.