Runtime: Disposing the FileSystemWatcher does not close associated file descriptors

[mrward]

Filed this against Mono, which uses corefx, but the problem also affects corefx.

https://github.com/mono/mono/issues/16709

Steps to Reproduce

Related problem - https://github.com/mono/mono/issues/15931

1. Sample project - https://github.com/mrward/test-file-watcher-dispose - multi-target project that builds .NET 4.7.2 and .NET Core 2.1
2. Build this project.
3. From the command line run dotnet bin/debug/netcoreapp2.1/FileWatcherDisposeTest.dll
4. Run lsof -p pid. Process id is output from the console app.
5. See that the file watcher creates file descriptors for several directories.
6. After 10 seconds the file watcher will be disposed.
7. Run lsof -p pid again.

Current Behavior

lsof still lists the directories.

dotnet 3972 user    4r DIR   1,12      170   51740695 /Volumes/Drive/projects/tests/FileWatcherDisposeTest/FileWatcherDisposeTest/bin/Debug/net472
dotnet 3972 user    5r DIR   1,12      136   51740600 /Volumes/Drive/projects/tests/FileWatcherDisposeTest/FileWatcherDisposeTest/bin/Debug
dotnet 3972 user    6r DIR   1,12      102   51740599 /Volumes/Drive/projects/tests/FileWatcherDisposeTest/FileWatcherDisposeTest/bin
dotnet 3972 user    7r DIR   1,12      204   51740594 /Volumes/Drive/projects/tests/FileWatcherDisposeTest/FileWatcherDisposeTest
dotnet 3972 user    8r DIR   1,12      170   51740593 /Volumes/Drive/projects/tests/FileWatcherDisposeTest
dotnet 3972 user    9r DIR   1,12     6324     185300 /Volumes/Drive/projects/tests
dotnet 3972 user   10r DIR   1,12     2312        278 /Volumes/Drive/projects
dotnet 3972 user   11r DIR   1,12      646          2 /Volumes/Drive
dotnet 3972 user   12r DIR    1,4      160     420284 /Volumes

If you change the console project to run GC.Collect after the dispose does not fix the problem (this does work with Mono)

Expected Behavior

File watcher directories not listed by lsof.

Note that

On which platforms did you notice this

[x] macOS
[ ] Linux
[ ] Windows

Version Used:

dotnet --info
.NET Core SDK (reflecting any global.json):
Version: 3.0.100-preview9-014004
Commit: 8e7ef240a5

Runtime Environment:
OS Name: Mac OS X
OS Version: 10.14
OS Platform: Darwin
RID: osx.10.14-x64
Base Path: /usr/local/share/dotnet/sdk/3.0.100-preview9-014004/

Host (useful for support):
Version: 3.0.0-preview9-19423-09
Commit: 2be172345a

.NET Core SDKs installed:
2.1.701 [/usr/local/share/dotnet/sdk]
3.0.100-preview9-014004 [/usr/local/share/dotnet/sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.12 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.12 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.0.0-preview9.19424.4 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.12 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.0-preview9-19423-09 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Stacktrace

[danmosemsft]

I don't know this code I just scanned it. But should this handle be explicitly disposed somewhere? @JeremyKuhne @carlossanlop

https://github.com/dotnet/corefx/blob/bca549851e2b24eceb1968a4a8ebab5f2ba4f7f2/src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs#L143

[scalablecory]

@wfurt was recently looking at FileSystemWatcher on OSX

[wfurt]

I can take a look as well I can run repro on Linux for comparison. Also note that there was issue with Sockets that in some cases underlying handle would not be properly closed. That would show up if you target 2.1 even if you use 3.0 SDK.

[marek-safar]

@danmosemsft @karelz this is impacting VS4M, it would be great to prioritize the investigation so we know what the options are for us.

[Therzok]

Profiling this, I found that the RunningInstance is being alive by these 2 paths. Sadly, there are no field names, as the mono profiler does not report field names.

[Therzok]

These both keep the EventStreamSafeHandle and EventStreamCallback instances alive, leading to the native one not being released.

[Therzok]

To note, the System.IO.CoreFX.FileSystemWatcher namespace is due to a change in mono:
https://github.com/mono/corefx/blob/master/src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs#L19-L24

[Therzok]

It's not just the safeHandle, the CancellationToken registration should also be disposed:
https://github.com/dotnet/corefx/blob/bca549851e2b24eceb1968a4a8ebab5f2ba4f7f2/src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs#L173

[danmosemsft]

@therzok if you modify the code to do that can you confirm it solves it?

[Therzok]

@danmosemsft Don't have cycles available to take a look at it right now, sorry!

[danmosemsft]

@Therzok not a problem, thanks for adding the information.

[slluis]

It would be great to have a quick fix for this. It has an important impact in VSMac.

[danmosemsft]

@slluis could you tell us more about the impact? Is this somehow a new problem?

What versions of .NET Core would VSMac require a fix in?

[mrward]

@danmosemsft The fix in corefx would be backported to Mono. Not sure what version of corefix Mono would be using. @steveisok may know more.

Currently this causes VS Mac to keep file descriptors around after file watchers are disposed. Eventually this breaks Mono's networking stack, which has a limit of 1024 file descriptors.

[wfurt]

I'll take a look. I drop it before as it look like @Therzok was digging into it.

[Therzok]

@wfurt just wanted to provide some assistance on the issue, as the retention paths should most likely be the same :smile:

[steveisok]

Mono is set up to track 2.2, so if a fix lands there it would be the easiest.

[wfurt]

I put up PR with proposed fix. I'm not sure if possible but it would be great if anybody can give it try with VS.
It does fix issue observed with provided repro.

[Therzok]

It fixes it!

[mrward]

The PR fixes the problem for console test app that uses one file watcher. If I create two or more then only the first seems to have its file descriptors closed.

VS Mac running with Mono 6.4.0.194 (2019-06/7da9a041b3b Thu Sep 12 04:09:42 EDT 2019) which has the fix still has problems here.

Will look at creating updating the sample project.

[mrward]

There is an updated test console project that creates more than file watcher - https://github.com/mrward/test-file-watcher-dispose

[Therzok]

Validated the corefx change against local corefx:
https://gist.github.com/Therzok/37489ee04505d5e1c4a2d43ca85abed9

Definitely fixed here, the rest of the problems are in mono. Thank you @wfurt!

[danmosemsft]

@steveisok can you confirm that VS for Mac does _not_ need this back ported in corefx (eg., to 3.0.1) since evidently VS for Mac is consuming it out of the Mono sources?

[steveisok]

Correct. We do not need it backported.

[wfurt]

the extra code is to make sure Dispose() works concurrently from multiple threads. That may not be VS/Mac use case. I also put more notes to dotnet/runtime#30415 (just in case it matters for VS use)

[danmosemsft]

@wfurt since VS for mac does not need a backport my suggestion is to close this and wait see whether we get customer feedback that justifies a backport. Taht also gives more time for it in master. If you agree -we can close this.

[wfurt]

should we get this to master @danmosemsft ? I was waiting for somebody from core team to review this before merging to master.

[danmosemsft]

Oh - yes for sure @wfurt. I missed that it wasn't in master yet.