Fix invoking of `FileSystemWatcher` #53151

thomsj · 2021-05-23T23:44:52Z

Call protected On... event raising methods from Notify... methods,
to invoke SynchronizingObject when required.

ghost · 2021-05-23T23:44:58Z

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Call protected On... event raising methods from Notify... methods,
to invoke SynchronizingObject when required.

Author:	thomsj
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

dnfadmin · 2021-05-23T23:45:06Z

All CLA requirements met.

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs

src/libraries/System.IO.FileSystem.Watcher/tests/Utility/TestFileSystemWatcher.cs

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs

thomsj

I have made the change you suggested and reverted, except I have removed the handler != null check, since the check is also in InvokeOn().

Please put back the null checks. We don't want to do work like allocating the event args or performing a name match if there's no handler registered. The extra null check won't matter.

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs

stephentoub · 2021-06-11T16:55:28Z

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

@@ -102,24 +102,24 @@ public void SynchronizingObject_CalledOnEvent(WatcherChangeTypes expectedChangeT
                if (expectedChangeType == WatcherChangeTypes.Created)
                {
                    watcher.Created += dele;
-                    watcher.CallOnCreated(new FileSystemEventArgs(WatcherChangeTypes.Created, "test", "name"));
+                    watcher.CallNotifyFileSystemEventArgs(WatcherChangeTypes.Created, "name");


These tests shouldn't be changing. They were validating the behavior for when the OnXx methods were explicitly called, which, for example, a type derived from FileSystemWatcher could do (that's why these are all using a TestFileSystemWatcher, which is a derived type that just exposes those On methods for use).

So all of these tests changes should be reverted. Instead, tests should be added that validate that actually making file changes which in turn trigger the FileSystemWatcher to invoke event handlers does so with those invocations in the right place.

stephentoub

Thanks for working on this. The product changes now look good. The tests need some further changes, as noted in my comments. Let us know if you need any assistance.

thomsj · 2021-06-14T22:48:33Z

Are the TestISynchronizeInvoke delegates, such as the following, supposed to be invoked?

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

Lines 121 to 137 in 6474c72

    
           /// <summary> 
        
           /// Ensure that the SynchronizeObject is invoked when an Renamed event occurs 
        
           /// </summary> 
        
           [Fact] 
        
           public void SynchronizingObject_CalledOnRenamed() 
        
           { 
        
               RenamedEventHandler dele = (sender, e) => { Assert.Equal(WatcherChangeTypes.Renamed, e.ChangeType); }; 
        
               TestISynchronizeInvoke invoker = new TestISynchronizeInvoke() { ExpectedDelegate = dele }; 
        
               using (var testDirectory = new TempDirectory(GetTestFilePath())) 
        
               using (var watcher = new TestFileSystemWatcher(testDirectory.Path, "*")) 
        
               { 
        
                   watcher.SynchronizingObject = invoker; 
        
                   watcher.Renamed += dele; 
        
                   watcher.CallOnRenamed(new RenamedEventArgs(WatcherChangeTypes.Changed, "test", "name", "oldname")); 
        
                   Assert.True(invoker.BeginInvoke_Called); 
        
               } 
        
           }

If so, should TestISynchronizeInvoke.BeginInvoke() be changed to something like the below?

public IAsyncResult BeginInvoke(Delegate method, object[] args)
{
    Assert.Equal(ExpectedDelegate, method);
    BeginInvoke_Called = true;
    method.DynamicInvoke(args[0], args[1]);
    return null;
}

thomsj · 2021-06-29T00:08:26Z

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

+            // block the handling thread
+            watcher.Changed += (o, e) => unblockHandler.WaitOne();


Will this work with a recreated watcher?

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

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?

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

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

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

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

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;

}

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

stephentoub · 2021-07-09T21:48:06Z

@thomsj, all fine questions, but most of them don't seem related to this PR. Are there specific questions related to this fix you still have? Thanks.

stephentoub · 2021-07-09T21:51:38Z

src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Move.cs

+        [Fact]
+        public void FileSystemWatcher_Directory_Move_SynchronizingObject()
+        {
+            TestISynchronizeInvoke invoker = new TestISynchronizeInvoke();


nit: for all of these tests, you can move this into the using block, just above where you store it into SynchronizingObject.

I've moved this for all the tests. I think I probably put it where I did and didn't use var because that's how it's done in tests/FileSystemWatcher.cs

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

thomsj · 2021-07-10T00:45:30Z

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

+            public IAsyncResult BeginInvoke(Delegate method, object[] args)
+            {
+                if (ExpectedDelegate != null)
+                    Assert.Equal(ExpectedDelegate, method);
+
+                BeginInvoke_Called = true;
+                method.DynamicInvoke(args[0], args[1]);
+                return null;
+            }


Are the TestISynchronizeInvoke delegates, such as the following, supposed to be invoked?

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

Lines 121 to 137 in 6474c72

/// <summary>

/// Ensure that the SynchronizeObject is invoked when an Renamed event occurs

/// </summary>

[Fact]

public void SynchronizingObject_CalledOnRenamed()

{

RenamedEventHandler dele = (sender, e) => { Assert.Equal(WatcherChangeTypes.Renamed, e.ChangeType); };

TestISynchronizeInvoke invoker = new TestISynchronizeInvoke() { ExpectedDelegate = dele };

using (var testDirectory = new TempDirectory(GetTestFilePath()))

using (var watcher = new TestFileSystemWatcher(testDirectory.Path, "*"))

{

watcher.SynchronizingObject = invoker;

watcher.Renamed += dele;

watcher.CallOnRenamed(new RenamedEventArgs(WatcherChangeTypes.Changed, "test", "name", "oldname"));

Assert.True(invoker.BeginInvoke_Called);

}

}

If so, should TestISynchronizeInvoke.BeginInvoke() be changed to something like the below?

public IAsyncResult BeginInvoke(Delegate method, object[] args) { Assert.Equal(ExpectedDelegate, method); BeginInvoke_Called = true; method.DynamicInvoke(args[0], args[1]); return null; }

I changed this to something like the above, so the following handler would get invoked:

https://github.com/dotnet/runtime/blob/9fa24a29c84ddb49492ca9ce7cfcc9bda82ff264/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs#L399-L403

thomsj · 2021-07-10T23:25:51Z

@thomsj, all fine questions, but most of them don't seem related to this PR. Are there specific questions related to this fix you still have? Thanks.

As you've noted, I've noticed a few things which aren't directly related to this PR, so my only question is, do you want me to remove anything from this PR, such as the changes to FileSystemWatcher.OSX.cs?

stephentoub · 2021-07-15T15:52:16Z

do you want me to remove anything from this PR, such as the changes to FileSystemWatcher.OSX.cs?

Yes, please, thanks. I'd like to keep this focused on the fix.

thomsj · 2021-07-19T21:51:49Z

do you want me to remove anything from this PR, such as the changes to FileSystemWatcher.OSX.cs?

Yes, please, thanks. I'd like to keep this focused on the fix.

I've reverted the FileSystemWatcher.OSX.cs changes.

stephentoub · 2021-07-20T02:57:14Z

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

+                    {
+                        _output?.WriteLine(ex.ToString());
+                        throw;
+                    }


Why is all of this stuff changing?

I've reverted the vast majority of the changes to this file.

danmoseley · 2021-08-09T23:02:58Z

@stephentoub is next action here on us or @thomsj

stephentoub · 2021-08-13T14:06:28Z

@stephentoub is next action here on us or @thomsj

It's on me.

Call `protected` `On...` event raising methods from `Notify...` methods, to invoke `SynchronizingObject` when required. Fix dotnet#52644

stephentoub · 2021-08-13T14:24:42Z

LGTM. Thanks, @thomsj. I rebased your branch on top of main.

thomsj · 2021-08-18T21:53:05Z

Thanks a lot for your guidance, @stephentoub.

stephentoub · 2021-08-19T00:38:28Z

Thanks for the fix :-)

dotnet-issue-labeler bot added the area-System.IO label May 23, 2021

thomsj commented May 24, 2021

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem.Watcher/tests/Utility/TestFileSystemWatcher.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 24, 2021

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs Outdated Show resolved Hide resolved

thomsj commented May 25, 2021

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Jun 2, 2021

Host tests often fail on text file busy issues #53587

Closed

stephentoub reviewed Jun 11, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

jozkee assigned thomsj Jun 16, 2021

jozkee added this to the 6.0.0 milestone Jun 16, 2021

This comment has been minimized.

Sign in to view

thomsj commented Jun 29, 2021

View reviewed changes

thomsj requested a review from stephentoub June 29, 2021 00:15

stephentoub reviewed Jul 9, 2021

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.InternalBufferSize.cs Outdated Show resolved Hide resolved

thomsj commented Jul 10, 2021

View reviewed changes

thomsj requested a review from stephentoub July 10, 2021 23:38

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

stephentoub reviewed Jul 20, 2021

View reviewed changes

karelz mentioned this pull request Jul 20, 2021

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - Assert ::1 #54677

Closed

karelz mentioned this pull request Jul 20, 2021

System.Net.Quic.Tests.QuicStreamTests_MockProvider.WriteCanceled_NextWriteThrows is failing on redhat.7.amd64 #55995

Closed

thomsj requested a review from stephentoub July 20, 2021 23:46

jeffhandley assigned stephentoub Jul 23, 2021

Fix invoking of FileSystemWatcher

d8706a1

Call `protected` `On...` event raising methods from `Notify...` methods, to invoke `SynchronizingObject` when required. Fix dotnet#52644

stephentoub force-pushed the fix-FileSystemWatcher-invoke branch from d07203f to d8706a1 Compare August 13, 2021 14:24

stephentoub approved these changes Aug 13, 2021

View reviewed changes

carlossanlop approved these changes Aug 13, 2021

View reviewed changes

carlossanlop merged commit 5817dc8 into dotnet:main Aug 13, 2021

ghost locked as resolved and limited conversation to collaborators Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invoking of `FileSystemWatcher` #53151

Fix invoking of `FileSystemWatcher` #53151

thomsj commented May 23, 2021

ghost commented May 23, 2021

dnfadmin commented May 23, 2021 •

edited

Loading

thomsj left a comment

stephentoub Jun 11, 2021

stephentoub left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

thomsj commented Jun 14, 2021

This comment has been minimized.

thomsj Jun 29, 2021

stephentoub Jul 9, 2021

thomsj Jul 9, 2021

stephentoub Jul 20, 2021

thomsj Jul 20, 2021

stephentoub Jul 21, 2021

stephentoub Jul 21, 2021

thomsj Jul 21, 2021

stephentoub commented Jul 9, 2021

stephentoub Jul 9, 2021

thomsj Jul 10, 2021

thomsj Jul 10, 2021

thomsj commented Jul 10, 2021

stephentoub commented Jul 15, 2021

thomsj commented Jul 19, 2021

stephentoub Jul 20, 2021

thomsj Jul 20, 2021

danmoseley commented Aug 9, 2021

stephentoub commented Aug 13, 2021

stephentoub commented Aug 13, 2021

thomsj commented Aug 18, 2021

stephentoub commented Aug 19, 2021

		// block the handling thread
		watcher.Changed += (o, e) => unblockHandler.WaitOne();

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

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

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

	/// /// <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;
	}

	/// <summary>
	/// Ensure that the SynchronizeObject is invoked when an Renamed event occurs
	/// </summary>
	[Fact]
	public void SynchronizingObject_CalledOnRenamed()
	{
	RenamedEventHandler dele = (sender, e) => { Assert.Equal(WatcherChangeTypes.Renamed, e.ChangeType); };
	TestISynchronizeInvoke invoker = new TestISynchronizeInvoke() { ExpectedDelegate = dele };
	using (var testDirectory = new TempDirectory(GetTestFilePath()))
	using (var watcher = new TestFileSystemWatcher(testDirectory.Path, "*"))
	{
	watcher.SynchronizingObject = invoker;
	watcher.Renamed += dele;
	watcher.CallOnRenamed(new RenamedEventArgs(WatcherChangeTypes.Changed, "test", "name", "oldname"));
	Assert.True(invoker.BeginInvoke_Called);
	}
	}

Fix invoking of FileSystemWatcher #53151

Fix invoking of FileSystemWatcher #53151

Conversation

thomsj commented May 23, 2021

ghost commented May 23, 2021

dnfadmin commented May 23, 2021 • edited Loading

thomsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

thomsj commented Jun 14, 2021

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomsj commented Jul 10, 2021

stephentoub commented Jul 15, 2021

thomsj commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Aug 9, 2021

stephentoub commented Aug 13, 2021

stephentoub commented Aug 13, 2021

thomsj commented Aug 18, 2021

stephentoub commented Aug 19, 2021

Fix invoking of `FileSystemWatcher` #53151

Fix invoking of `FileSystemWatcher` #53151

dnfadmin commented May 23, 2021 •

edited

Loading