close _eventStream when stopping watcher #41013

wfurt · 2019-09-11T05:41:30Z

When even stream is created it needs to be properly cleaned up in order to close associated file handles and associated resources.
https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/FSEvents_ProgGuide/UsingtheFSEventsFramework/UsingtheFSEventsFramework.html

We need to call FSEventStreamInvalidate and FSEventStreamRelease. Conveniently, it seems like this is all encapsulated in SafeEventStreamHandle's ReleaseHandle.

I run repro code from #40888 in loop and I did not see any descriptor leak. (The leak was pretty obvious without the fix)
I may spent more time tomorrow thinking if there is some good way how to add test but it may be tricky to verify OS resources.

fixes #40888

Therzok · 2019-09-11T11:48:25Z

You can probably use CFGetRetainCount before releasing the last ref to validate it's 1.

Backport of: dotnet#41013

(#343) Backport of: dotnet#41013

Therzok · 2019-09-11T17:16:33Z

Can confirm, mono side is fixed too 🎉

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

danmoseley · 2019-09-12T16:46:48Z

For 2.2 we'll need the usual PR with template, and mail to netcoreship. Thanks @wfurt.

wfurt · 2019-09-16T17:21:45Z

test failures in System.Security.Cryptography are unrelated. Please let me know if there is anything outstanding @JeremyKuhne and @stephentoub.
I would like to get this into master and work on 2.x port.

danmoseley · 2019-09-16T22:12:39Z

@carlossanlop perhaps you can review?

carlossanlop · 2019-09-27T20:41:37Z

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

            {
                _cancellation = null;
                token.Cancel();
+                token.Dispose();


@wfurt I have a question: why do we only cancel and dispose the token here? The RunningInstance class received a copy of this token in its constructor, so why not perform these two actions inside CancellationCallback too? (follow up of Marius' question).

CancellationCallback() did not do anything with the token. It is StartRaisingEvents() where token was created and symmetrically StopRaisingEvents() where token is destroyed. Existing _cancellation = null should be sufficient IMHO to dispose token. I only add explicit Dispose() call to make it explicit and not wait for GC. I did not want to introduce different life cycle for the token and worry about what changed.

carlossanlop

Left a question.
Would it make sense to add unit tests to verify this change?
The build failed. Do you know what was the cause?

stephentoub · 2019-10-21T15:46:56Z

@wfurt, what's the status of this PR? Can it be made ready to merge, or closed?

wfurt · 2019-10-21T22:54:44Z

Tests are failing in System.Security.Cryptography on OSX and x86 Windows. That seems unrelated to this change.

As far as the test I could not come up with one I would like. Best I think I can do is to create and destroy watcher in loop and make loop count bigger than default handle limit. But CI does manipulate limits AFAIK and well as that may differ across systems. Also tests trying to exhaust system resources are potentially dangerous. Let me know @carlossanlop if you have suggestions.

carlossanlop · 2019-10-24T17:38:35Z

As far as the test I could not come up with one I would like. Best I think I can do is to create and destroy watcher in loop and make loop count bigger than default handle limit. But CI does manipulate limits AFAIK and well as that may differ across systems. Also tests trying to exhaust system resources are potentially dangerous. Let me know @carlossanlop if you have suggestions.

I noticed the original issue #40888 has a small file watcher test here: https://github.com/mrward/test-file-watcher-dispose/blob/master/Program.cs

Would it make sense to write a unit test that looks somewhat like in that code, or would we encounter the CI limit you mentioned, @wfurt ?

Therzok · 2019-10-24T17:47:31Z

I know this is icky, but what if we had something like:

var streamHandle = // use reflection to get the native handle
var ref = new WeakReference (streamHandle);
...
// do the test

Assert.AreEqual (1, CFRetainCount (streamHandle.Handle));

// dispose the watcher and wait for GC to run

Assert.IsNull (ref.Target);

…er_40887

wfurt · 2019-10-24T20:10:29Z

I added test and verified it fails in CI without the fix same way as #40888
https://helix.dot.net/api/2019-06-17/jobs/7d88982f-e479-45e6-b82f-36c65c3e89bd/workitems/System.IO.FileSystem.Watcher.Tests/console

   System.IO.Tests.FileSystemWatcherTests_Unix.Watcher_Usage_DoesNotLeak [STARTING]
Limits are 10240 and 18446744073709551615
File open failed at 10067 with Too many open files
    System.IO.Tests.FileSystemWatcherTests_Unix.Watcher_Usage_DoesNotLeak [FINISHED] Time: 0.6855082s

I use new trick to push test to the and making sure that it does not run with any other tests.
That allows to use XUnit output helper . (which is not available for "remote" exec in separate process.

wfurt · 2019-10-24T23:25:30Z

/azp run

azure-pipelines · 2019-10-24T23:25:50Z

Azure Pipelines successfully started running 4 pipeline(s).

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

carlossanlop

@wfurt I left a small nit comment.
Also, there were failures in musl and in outerloop. Would you mind taking a look?

wfurt · 2019-11-04T22:19:20Z

/azp run

azure-pipelines · 2019-11-04T22:19:44Z

Azure Pipelines successfully started running 4 pipeline(s).

wfurt · 2019-11-05T06:33:56Z

/azp run

azure-pipelines · 2019-11-05T06:34:18Z

Azure Pipelines successfully started running 4 pipeline(s).

wfurt · 2019-11-05T19:13:18Z

System.Runtime tests failures are unrelated. They also fail in master. The Musl run did not producer any reasonable output., That seems like infrastructure problem.

maryamariyan · 2019-11-06T21:10:02Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your corefx repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

carlossanlop

Thanks for checking the unit tests. I don't have any more input.

close _eventStream when stopping watcher

c0f2178

wfurt added the area-System.IO label Sep 11, 2019

wfurt requested review from JeremyKuhne, carlossanlop and stephentoub September 11, 2019 05:41

wfurt self-assigned this Sep 11, 2019

steveisok pushed a commit to steveisok/corefx that referenced this pull request Sep 11, 2019

Pull in fsw fix that fixes https://github.com/dotnet/corefx/issues/40888

5b5dd88

Backport of: dotnet#41013

steveisok mentioned this pull request Sep 11, 2019

Pull in fsw fix that fixes https://github.com/dotnet/corefx/issues/40888 mono/corefx#343

Merged

steveisok added a commit to mono/corefx that referenced this pull request Sep 11, 2019

Pull in fsw fix that fixes https://github.com/dotnet/corefx/issues/40888

e79cf5b

(#343) Backport of: dotnet#41013

Therzok approved these changes Sep 11, 2019

View reviewed changes

stephentoub reviewed Sep 11, 2019

View reviewed changes

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

danmoseley added this to the 5.0 milestone Sep 12, 2019

feedback from review

1999648

carlossanlop reviewed Sep 27, 2019

View reviewed changes

carlossanlop suggested changes Sep 27, 2019

View reviewed changes

maryamariyan assigned JeremyKuhne and carlossanlop and unassigned wfurt Oct 7, 2019

carlossanlop assigned wfurt and unassigned carlossanlop and JeremyKuhne Oct 7, 2019

wfurt added 3 commits October 24, 2019 12:52

pull in ResourceLimits

19b0500

Merge branch 'master' of https://github.com./dotnet/corefx into watch…

7a8772d

…er_40887

add test for leaking handle

d325a2f

maryamariyan requested a review from carlossanlop November 1, 2019 16:46

carlossanlop reviewed Nov 4, 2019

View reviewed changes

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

carlossanlop suggested changes Nov 4, 2019

View reviewed changes

fix typo

ecf973b

force run for finalizers

fd3a50f

carlossanlop approved these changes Nov 7, 2019

View reviewed changes

wfurt merged commit dd4fa0d into dotnet:master Nov 7, 2019

wfurt deleted the watcher_40888 branch November 7, 2019 02:09

close _eventStream when stopping watcher #41013

close _eventStream when stopping watcher #41013

Uh oh!

Conversation

wfurt commented Sep 11, 2019

Uh oh!

Therzok commented Sep 11, 2019

Uh oh!

Therzok commented Sep 11, 2019

Uh oh!

Uh oh!

danmoseley commented Sep 12, 2019

Uh oh!

wfurt commented Sep 16, 2019

Uh oh!

danmoseley commented Sep 16, 2019

Uh oh!

carlossanlop Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Oct 21, 2019

Uh oh!

wfurt commented Oct 21, 2019

Uh oh!

carlossanlop commented Oct 24, 2019

Uh oh!

Therzok commented Oct 24, 2019

Uh oh!

wfurt commented Oct 24, 2019

Uh oh!

wfurt commented Oct 24, 2019

Uh oh!

azure-pipelines bot commented Oct 24, 2019

Uh oh!

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

wfurt commented Nov 4, 2019

Uh oh!

azure-pipelines bot commented Nov 4, 2019

Uh oh!

wfurt commented Nov 5, 2019

Uh oh!

azure-pipelines bot commented Nov 5, 2019

Uh oh!

wfurt commented Nov 5, 2019

Uh oh!

maryamariyan commented Nov 6, 2019

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

carlossanlop Sep 27, 2019 •

edited

Loading