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

Fix invoking of FileSystemWatcher #53151

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Fix invoking of FileSystemWatcher #53151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 9 additions & 7 deletions src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,11 @@ private bool MatchPattern(ReadOnlySpan<char> relativePath)
/// </summary>
private void NotifyInternalBufferOverflowEvent()
{
_onErrorHandler?.Invoke(this, new ErrorEventArgs(
new InternalBufferOverflowException(SR.Format(SR.FSW_BufferOverflow, _directory))));
if (_onErrorHandler != null)
{
OnError(new ErrorEventArgs(
new InternalBufferOverflowException(SR.Format(SR.FSW_BufferOverflow, _directory))));
}
}

/// <summary>
Expand All @@ -418,11 +421,10 @@ private void NotifyInternalBufferOverflowEvent()
private void NotifyRenameEventArgs(WatcherChangeTypes action, ReadOnlySpan<char> name, ReadOnlySpan<char> oldName)
{
// filter if there's no handler or neither new name or old name match a specified pattern
RenamedEventHandler? handler = _onRenamedHandler;
if (handler != null &&
if (_onRenamedHandler != null &&
(MatchPattern(name) || MatchPattern(oldName)))
{
handler(this, new RenamedEventArgs(action, _directory, name.IsEmpty ? null : name.ToString(), oldName.IsEmpty ? null : oldName.ToString()));
OnRenamed(new RenamedEventArgs(action, _directory, name.IsEmpty ? null : name.ToString(), oldName.IsEmpty ? null : oldName.ToString()));
}
}

Expand Down Expand Up @@ -451,7 +453,7 @@ private void NotifyFileSystemEventArgs(WatcherChangeTypes changeType, ReadOnlySp

if (handler != null && MatchPattern(name.IsEmpty ? _directory : name))
{
handler(this, new FileSystemEventArgs(changeType, _directory, name.IsEmpty ? null : name.ToString()));
InvokeOn(new FileSystemEventArgs(changeType, _directory, name.IsEmpty ? null : name.ToString()), handler);
}
}

Expand All @@ -464,7 +466,7 @@ private void NotifyFileSystemEventArgs(WatcherChangeTypes changeType, string nam

if (handler != null && MatchPattern(string.IsNullOrEmpty(name) ? _directory : name))
{
handler(this, new FileSystemEventArgs(changeType, _directory, name));
InvokeOn(new FileSystemEventArgs(changeType, _directory, name), handler);
}
}

Expand Down
17 changes: 17 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Changed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,22 @@ public void FileSystemWatcher_Directory_Changed_SymLink()
ExpectEvent(watcher, 0, action, cleanup, dir.Path);
}
}

[Fact]
public void FileSystemWatcher_Directory_Changed_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var dir = new TempDirectory(Path.Combine(testDirectory.Path, "dir")))
using (var watcher = new FileSystemWatcher(testDirectory.Path, Path.GetFileName(dir.Path)))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

Action action = () => Directory.SetLastWriteTime(dir.Path, DateTime.Now + TimeSpan.FromSeconds(10));

ExpectEvent(watcher, WatcherChangeTypes.Changed, action, expectedPath: dir.Path);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
20 changes: 20 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Create.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,25 @@ public void FileSystemWatcher_Directory_Create_SymLink()
ExpectEvent(watcher, WatcherChangeTypes.Created, action, cleanup, symLinkPath);
}
}

[Fact]
public void FileSystemWatcher_Directory_Create_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var watcher = new FileSystemWatcher(testDirectory.Path))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string dirName = Path.Combine(testDirectory.Path, "dir");
watcher.Filter = Path.GetFileName(dirName);

Action action = () => Directory.CreateDirectory(dirName);
Action cleanup = () => Directory.Delete(dirName);

ExpectEvent(watcher, WatcherChangeTypes.Created, action, cleanup, dirName);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
21 changes: 21 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Delete.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,26 @@ public void FileSystemWatcher_Directory_Delete_SymLink()
ExpectEvent(watcher, WatcherChangeTypes.Deleted, action, cleanup, expectedPath: symLinkPath);
}
}

[Fact]
public void FileSystemWatcher_Directory_Delete_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var watcher = new FileSystemWatcher(testDirectory.Path))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string dirName = Path.Combine(testDirectory.Path, "dir");
watcher.Filter = Path.GetFileName(dirName);

Action action = () => Directory.Delete(dirName);
Action cleanup = () => Directory.CreateDirectory(dirName);
cleanup();

ExpectEvent(watcher, WatcherChangeTypes.Deleted, action, cleanup, dirName);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
21 changes: 21 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Move.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,27 @@ public void Directory_Move_With_Set_NotifyFilter()
DirectoryMove_WithNotifyFilter(WatcherChangeTypes.Renamed);
}

[Fact]
public void Directory_Move_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var dir = new TempDirectory(Path.Combine(testDirectory.Path, "dir")))
using (var watcher = new FileSystemWatcher(testDirectory.Path))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string sourcePath = dir.Path;
string targetPath = Path.Combine(testDirectory.Path, "target");

Action action = () => Directory.Move(sourcePath, targetPath);
Action cleanup = () => Directory.Move(targetPath, sourcePath);

ExpectEvent(watcher, WatcherChangeTypes.Renamed, action, cleanup, targetPath);
Assert.True(invoker.BeginInvoke_Called);
}
}

#region Test Helpers

private void DirectoryMove_SameDirectory(WatcherChangeTypes eventType)
Expand Down
17 changes: 17 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Changed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,22 @@ public void FileSystemWatcher_File_Changed_SymLink()
ExpectEvent(watcher, 0, action, cleanup, expectedPath: file.Path);
}
}

[Fact]
public void FileSystemWatcher_File_Changed_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var file = new TempFile(Path.Combine(testDirectory.Path, "file")))
using (var watcher = new FileSystemWatcher(testDirectory.Path, Path.GetFileName(file.Path)))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

Action action = () => Directory.SetLastWriteTime(file.Path, DateTime.Now + TimeSpan.FromSeconds(10));

ExpectEvent(watcher, WatcherChangeTypes.Changed, action, expectedPath: file.Path);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
20 changes: 20 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Create.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,5 +141,25 @@ public void FileSystemWatcher_File_Create_SymLink()
ExpectEvent(watcher, WatcherChangeTypes.Created, action, cleanup, symLinkPath);
}
}

[Fact]
public void FileSystemWatcher_File_Create_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var watcher = new FileSystemWatcher(testDirectory.Path))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string fileName = Path.Combine(testDirectory.Path, "file");
watcher.Filter = Path.GetFileName(fileName);

Action action = () => File.Create(fileName).Dispose();
Action cleanup = () => File.Delete(fileName);

ExpectEvent(watcher, WatcherChangeTypes.Created, action, cleanup, fileName);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
21 changes: 21 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Delete.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,26 @@ public void FileSystemWatcher_File_Delete_SymLink()
ExpectEvent(watcher, WatcherChangeTypes.Deleted, action, cleanup, symLinkPath);
}
}

[Fact]
public void FileSystemWatcher_File_Delete_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var watcher = new FileSystemWatcher(testDirectory.Path))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string fileName = Path.Combine(testDirectory.Path, "file");
watcher.Filter = Path.GetFileName(fileName);

Action action = () => File.Delete(fileName);
Action cleanup = () => File.Create(fileName).Dispose();
cleanup();

ExpectEvent(watcher, WatcherChangeTypes.Deleted, action, cleanup, fileName);
Assert.True(invoker.BeginInvoke_Called);
}
}
}
}
23 changes: 23 additions & 0 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,29 @@ public void Unix_File_Move_With_Set_NotifyFilter()
FileMove_WithNotifyFilter(WatcherChangeTypes.Renamed);
}

[Fact]
public void File_Move_SynchronizingObject()
{
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var dir = new TempDirectory(Path.Combine(testDirectory.Path, "dir")))
using (var testFile = new TempFile(Path.Combine(dir.Path, "file")))
using (var watcher = new FileSystemWatcher(dir.Path, "*"))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

string sourcePath = testFile.Path;
string targetPath = testFile.Path + "_Renamed";

// Move the testFile to a different name in the same directory
Action action = () => File.Move(sourcePath, targetPath);
Action cleanup = () => File.Move(targetPath, sourcePath);

ExpectEvent(watcher, WatcherChangeTypes.Renamed, action, cleanup, targetPath);
Assert.True(invoker.BeginInvoke_Called);
}
}

#region Test Helpers

private void FileMove_SameDirectory(WatcherChangeTypes eventType)
Expand Down
80 changes: 60 additions & 20 deletions src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.InternalBufferSize.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,40 +44,80 @@ public class InternalBufferSizeTests : FileSystemWatcherTest
[PlatformSpecific(TestPlatforms.Windows)] // Uses P/Invokes
public void FileSystemWatcher_InternalBufferSize(bool setToHigherCapacity)
{
ManualResetEvent unblockHandler = new ManualResetEvent(false);
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var file = new TempFile(Path.Combine(testDirectory.Path, "file")))
using (var watcher = new FileSystemWatcher(testDirectory.Path, Path.GetFileName(file.Path)))
using (FileSystemWatcher watcher = CreateWatcher(testDirectory.Path, file.Path, unblockHandler))
{
int internalBufferOperationCapacity = watcher.InternalBufferSize / (17 + Path.GetFileName(file.Path).Length);
int internalBufferOperationCapacity = CalculateInternalBufferOperationCapacity(watcher.InternalBufferSize, file.Path);

// Set the capacity high to ensure no error events arise.
if (setToHigherCapacity)
watcher.InternalBufferSize = watcher.InternalBufferSize * 12;

// block the handling thread
ManualResetEvent unblockHandler = new ManualResetEvent(false);
watcher.Changed += (o, e) =>
{
unblockHandler.WaitOne();
};

// generate enough file change events to overflow the default buffer
Action action = () =>
{
for (int i = 1; i < internalBufferOperationCapacity * 10; i++)
{
File.SetLastWriteTime(file.Path, DateTime.Now + TimeSpan.FromSeconds(i));
}

unblockHandler.Set();
};
Action cleanup = () => unblockHandler.Reset();
Action action = GetAction(unblockHandler, internalBufferOperationCapacity, file.Path);
Action cleanup = GetCleanup(unblockHandler);

if (setToHigherCapacity)
ExpectNoError(watcher, action, cleanup);
else
ExpectError(watcher, action, cleanup);
}
}

[Fact]
[PlatformSpecific(TestPlatforms.Windows)]
public void FileSystemWatcher_InternalBufferSize_SynchronizingObject()
{
ManualResetEvent unblockHandler = new ManualResetEvent(false);
using (var testDirectory = new TempDirectory(GetTestFilePath()))
using (var file = new TempFile(Path.Combine(testDirectory.Path, "file")))
using (FileSystemWatcher watcher = CreateWatcher(testDirectory.Path, file.Path, unblockHandler))
{
TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();
watcher.SynchronizingObject = invoker;

int internalBufferOperationCapacity = CalculateInternalBufferOperationCapacity(watcher.InternalBufferSize, file.Path);

Action action = GetAction(unblockHandler, internalBufferOperationCapacity, file.Path);
Action cleanup = GetCleanup(unblockHandler);

ExpectError(watcher, action, cleanup);
Assert.True(invoker.BeginInvoke_Called);
}
}

#region Test Helpers

private FileSystemWatcher CreateWatcher(string testDirectoryPath, string filePath, ManualResetEvent unblockHandler)
{
var watcher = new FileSystemWatcher(testDirectoryPath, Path.GetFileName(filePath));

// block the handling thread
watcher.Changed += (o, e) => unblockHandler.WaitOne();
Comment on lines +96 to +97
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this work with a recreated watcher?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the question. Can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/f044460da04f0833990325b144f8f26478293cca/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs#L387-L425

https://github.com/dotnet/runtime/blob/f044460da04f0833990325b144f8f26478293cca/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs#L511-L528

If watcher is created in RecreateWatcher(), will the correct thread be blocked?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no. The new watcher you're creating won't have the event handler that was originally stored into the first watcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume I should leave this as is, since this is a pre-existing flaw with FileSystemWatcher_InternalBufferSize()?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can clean up stuff later if it's an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Except, is this actually pre-existing? Where is the existing FileSystemWatcher_InternalBufferSize recreating the watcher and re-setting this handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the main branch, FileSystemWatcher_InternalBufferSize() calls one of the test methods:

runtime/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.InternalBufferSize.cs

Lines 76 to 79 in 016851a

if (setToHigherCapacity)
ExpectNoError(watcher, action, cleanup);
else
ExpectError(watcher, action, cleanup);

Both of them call TryErrorEvent():

runtime/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs

Lines 351 to 355 in 016851a

public static void ExpectError(FileSystemWatcher watcher, Action action, Action cleanup, int attempts = DefaultAttemptsForExpectedEvent)
{
string message = string.Format("Did not observe an error event within {0}ms and {1} attempts.", WaitForExpectedEventTimeout, attempts);
Assert.True(TryErrorEvent(watcher, action, cleanup, attempts, expected: true), message);
}

runtime/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs

Lines 364 to 368 in 016851a

public static void ExpectNoError(FileSystemWatcher watcher, Action action, Action cleanup, int attempts = DefaultAttemptsForUnExpectedEvent)
{
string message = string.Format("Should not observe an error event within {0}ms. Attempted {1} times and received the event each time.", WaitForExpectedEventTimeout, attempts);
Assert.False(TryErrorEvent(watcher, action, cleanup, attempts, expected: true), message);
}

TryErrorEvent() recreates the watcher, but the handler is never re-set:

runtime/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs

Lines 370 to 431 in 016851a

/// /// <summary>
/// Helper method for the ExpectError/ExpectNoError functions.
/// </summary>
/// <param name="watcher">The FileSystemWatcher to test</param>
/// <param name="action">The Action to execute.</param>
/// <param name="cleanup">Undoes the action and cleans up the watcher so the test may be run again if necessary.</param>
/// <param name="attempts">Number of times the test should be executed if it's failing.</param>
/// <param name="expected">Whether it is expected that an error event will be arisen.</param>
/// <returns>True if an Error event was raised by the watcher when the given action was executed; else, false.</returns>
public static bool TryErrorEvent(FileSystemWatcher watcher, Action action, Action cleanup, int attempts, bool expected)
{
int attemptsCompleted = 0;
bool result = !expected;
while (result != expected && attemptsCompleted++ < attempts)
{
if (attemptsCompleted > 1)
{
// Re-create the watcher to get a clean iteration.
watcher = new FileSystemWatcher()
{
IncludeSubdirectories = watcher.IncludeSubdirectories,
NotifyFilter = watcher.NotifyFilter,
Filter = watcher.Filter,
Path = watcher.Path,
InternalBufferSize = watcher.InternalBufferSize
};
// Most intermittent failures in FSW are caused by either a shortage of resources (e.g. inotify instances)
// or by insufficient time to execute (e.g. CI gets bogged down). Immediately re-running a failed test
// won't resolve the first issue, so we wait a little while hoping that things clear up for the next run.
Thread.Sleep(500);
}
AutoResetEvent errorOccurred = new AutoResetEvent(false);
watcher.Error += (o, e) =>
{
errorOccurred.Set();
};
// Enable raising events but be careful with the possibility of the max user inotify instances being reached already.
if (attemptsCompleted <= attempts)
{
try
{
watcher.EnableRaisingEvents = true;
}
catch (IOException) // Max User INotify instances. Isn't the type of error we're checking for.
{
continue;
}
}
else
{
watcher.EnableRaisingEvents = true;
}
action();
result = errorOccurred.WaitOne(WaitForExpectedEventTimeout);
watcher.EnableRaisingEvents = false;
cleanup();
}
return result;
}


return watcher;
}

private int CalculateInternalBufferOperationCapacity(int internalBufferSize, string filePath) =>
internalBufferSize / (17 + Path.GetFileName(filePath).Length);

private Action GetAction(ManualResetEvent unblockHandler, int internalBufferOperationCapacity, string filePath)
{
return () =>
{
// generate enough file change events to overflow the default buffer
for (int i = 1; i < internalBufferOperationCapacity * 10; i++)
{
File.SetLastWriteTime(filePath, DateTime.Now + TimeSpan.FromSeconds(i));
}

unblockHandler.Set();
};
}

private Action GetCleanup(ManualResetEvent unblockHandler) => () => unblockHandler.Reset();

#endregion
}
}
Loading