Skip to content

Commit ad3da86

Browse files
committed
Addressed Copilot review comments
1 parent 2d57d0a commit ad3da86

2 files changed

Lines changed: 41 additions & 17 deletions

File tree

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ public class PhysicalFilesWatcher : IDisposable
3939
// inside _root (which is below the FSW's watched path) are observed.
4040
private readonly bool _fileWatcherIsAboveRoot;
4141
// Number of currently registered tokens whose pattern requires watching subdirectories.
42-
// Maintained as tokens are added and removed so we don't iterate the lookups.
42+
// Maintained as tokens are added and removed so we don't iterate the lookups when
43+
// re-evaluating IncludeSubdirectories.
4344
private int _subdirectoryRequiringTokenCount;
4445

4546
// A single non-recursive watcher used when _root does not exist.
@@ -127,9 +128,6 @@ public PhysicalFilesWatcher(
127128
}
128129

129130
_fileWatcher = fileSystemWatcher;
130-
// IncludeSubdirectories is set in TryEnableFileSystemWatcher based on whether
131-
// any registered token's pattern actually requires watching subdirectories.
132-
_fileWatcher.IncludeSubdirectories = false;
133131
_fileWatcher.Created += OnChanged;
134132
_fileWatcher.Changed += OnChanged;
135133
_fileWatcher.Renamed += OnRenamed;
@@ -389,7 +387,8 @@ void CancelAll(ConcurrentDictionary<string, ChangeTokenInfo> tokens, Func<string
389387
{
390388
if (requiresSubdirectories(entry.Key))
391389
{
392-
Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
390+
int newCount = Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
391+
Debug.Assert(newCount >= 0, "Subdirectory-requiring token counter went negative.");
393392
}
394393

395394
CancelToken(matchInfo);
@@ -454,7 +453,8 @@ private void ReportChangeForMatchedEntries(string path)
454453
{
455454
if (FilePathRequiresSubdirectories(path))
456455
{
457-
Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
456+
int newCount = Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
457+
Debug.Assert(newCount >= 0, "Subdirectory-requiring token counter went negative.");
458458
}
459459

460460
CancelToken(matchInfo);
@@ -469,7 +469,8 @@ private void ReportChangeForMatchedEntries(string path)
469469
{
470470
if (WildcardRequiresSubdirectories(wildCardEntry.Key))
471471
{
472-
Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
472+
int newCount = Interlocked.Decrement(ref _subdirectoryRequiringTokenCount);
473+
Debug.Assert(newCount >= 0, "Subdirectory-requiring token counter went negative.");
473474
}
474475

475476
CancelToken(matchInfo);
@@ -535,6 +536,14 @@ private void TryDisableFileSystemWatcher()
535536
// Perf: Turn off the file monitoring if no files or directories to monitor.
536537
_fileWatcher.EnableRaisingEvents = false;
537538
}
539+
else if (_fileWatcher.IncludeSubdirectories &&
540+
!_fileWatcherIsAboveRoot &&
541+
Volatile.Read(ref _subdirectoryRequiringTokenCount) == 0)
542+
{
543+
// Perf: Some tokens were removed and none of the remaining ones require
544+
// subdirectory watching, so we can stop recursing.
545+
_fileWatcher.IncludeSubdirectories = false;
546+
}
538547
}
539548
}
540549

src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFilesWatcherTests.cs

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -119,14 +119,13 @@ public void IncludeSubdirectories_DowngradedWhenSubdirectoryPatternRemoved()
119119
using var fileSystemWatcher = new MockFileSystemWatcher(root.Path);
120120
using var physicalFilesWatcher = new PhysicalFilesWatcher(root.Path, fileSystemWatcher, pollForChanges: false);
121121

122+
physicalFilesWatcher.CreateFileChangeToken("appsettings.json");
122123
physicalFilesWatcher.CreateFileChangeToken("sub/file.json");
123124
Assert.True(fileSystemWatcher.IncludeSubdirectories);
124125

125126
// Fire an event matching the subdirectory token to remove it from the lookup.
127+
// The remaining root-only token does not require subdirectory watching, IncludeSubdirectories should be downgraded.
126128
fileSystemWatcher.CallOnChanged(new FileSystemEventArgs(WatcherChangeTypes.Changed, root.Path, "sub/file.json"));
127-
128-
// Adding a new root-only token re-evaluates IncludeSubdirectories.
129-
physicalFilesWatcher.CreateFileChangeToken("appsettings.json");
130129
Assert.False(fileSystemWatcher.IncludeSubdirectories);
131130
}
132131

@@ -145,10 +144,6 @@ public void IncludeSubdirectories_NotDowngradedWhileSubdirectoryPatternRemains()
145144
// Remove only the root-level token; the subdirectory token must keep subdirectories enabled.
146145
fileSystemWatcher.CallOnChanged(new FileSystemEventArgs(WatcherChangeTypes.Changed, root.Path, "appsettings.json"));
147146
Assert.True(fileSystemWatcher.IncludeSubdirectories);
148-
149-
// Trigger re-evaluation by adding another root-only token.
150-
physicalFilesWatcher.CreateFileChangeToken("other.json");
151-
Assert.True(fileSystemWatcher.IncludeSubdirectories);
152147
}
153148

154149
[Fact]
@@ -166,9 +161,6 @@ public void IncludeSubdirectories_NotDowngradedWhenWildcardSubdirectoryPatternRe
166161
// Remove the root-level token; the recursive wildcard token must keep subdirectories enabled.
167162
fileSystemWatcher.CallOnChanged(new FileSystemEventArgs(WatcherChangeTypes.Changed, root.Path, "appsettings.json"));
168163
Assert.True(fileSystemWatcher.IncludeSubdirectories);
169-
170-
physicalFilesWatcher.CreateFileChangeToken("other.json");
171-
Assert.True(fileSystemWatcher.IncludeSubdirectories);
172164
}
173165

174166
[Fact]
@@ -188,6 +180,29 @@ public void IncludeSubdirectories_AlwaysTrueWhenWatcherIsAboveRoot()
188180
Assert.True(fsw.IncludeSubdirectories);
189181
}
190182

183+
[Fact]
184+
[SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.tvOS, "System.IO.FileSystem.Watcher is not supported on Browser/iOS/tvOS")]
185+
public async Task IncludeSubdirectories_StarPatternIsNotRecursive_DoesNotMatchSubdirectoryFile()
186+
{
187+
using var root = new TempDirectory(GetTestFilePath());
188+
using var fileSystemWatcher = new MockFileSystemWatcher(root.Path);
189+
using var physicalFilesWatcher = new PhysicalFilesWatcher(root.Path, fileSystemWatcher, pollForChanges: false);
190+
191+
IChangeToken token = physicalFilesWatcher.CreateFileChangeToken("*.json");
192+
Assert.False(fileSystemWatcher.IncludeSubdirectories);
193+
194+
Task changed = WhenChanged(token);
195+
196+
// A '*' pattern should not match files in subdirectories.
197+
fileSystemWatcher.CallOnChanged(new FileSystemEventArgs(WatcherChangeTypes.Changed, root.Path, "sub/foo.json"));
198+
await Task.Delay(WaitTimeForTokenToFire);
199+
Assert.False(changed.IsCompleted, "Token must not fire for an event in a subdirectory.");
200+
201+
// Sanity check: a root-level event does fire the token.
202+
fileSystemWatcher.CallOnChanged(new FileSystemEventArgs(WatcherChangeTypes.Changed, root.Path, "foo.json"));
203+
await changed;
204+
}
205+
191206
[Fact]
192207
[SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.tvOS, "System.IO.FileSystem.Watcher is not supported on Browser/iOS/tvOS")]
193208
public void CreateFileChangeToken_DoesNotAllowPathsAboveRoot()

0 commit comments

Comments
 (0)