Add missing FileSystemWatcher methods by ianhays · Pull Request #13656 · dotnet/corefx

ianhays · 2016-11-14T23:14:13Z

Adds source and tests for the remaining missing members in FileSystemWatcher. This required that FileSystemWatcher take a dependency on System.ComponentModel.TypeConverter.

resolves https://github.com/dotnet/corefx/issues/13491

@AlexGhiondea @danmosemsft @joperezr @alexperovich @JeremyKuhne

danmoseley · 2016-11-14T23:29:29Z

base.Dispose(disposing)
in desktop

Refers to: src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs:386 in 595aa0d. [](commit_id = 595aa0d, deletion_comment = False)

danmoseley · 2016-11-14T23:26:56Z

        protected void OnRenamed(RenamedEventArgs e)
        {
-            _onRenamedHandler?.Invoke(this, e);
+            RenamedEventHandler handler = _onRenamedHandler;


InvokeOn(e, _onRenamedHandler);

danmoseley · 2016-11-14T23:27:08Z

@@ -473,7 +496,14 @@ protected void OnDeleted(FileSystemEventArgs e)
        [SuppressMessage("Microsoft.Security", "CA2109:ReviewVisibleEventHandlers", MessageId = "0#", Justification = "Changing from protected to private would be a breaking change")]
        protected void OnError(ErrorEventArgs e)


InvokeOn(e, _onErrorHandler);

I didn't use the InvokeOn helper for Renamed and Error because of the different handler/args types that do not share a common parent.

danmoseley · 2016-11-14T23:27:29Z

+        public void EndInit()
+        {
+            _initializing = false;
+            //Start listening to events if _enabled was set to true at some point.


nit: space after //

danmoseley · 2016-11-14T23:36:34Z

+            }
+        }
+
+        /// <summary>


The name/comment of this test and the one below are switched

stephentoub · 2016-11-15T01:32:04Z

            _enabled = false;

+            if (IsSuspended())
+                return;


When will the subsequent logic be invoked in this case?

It won't be. When under initialization, the only action StopRaisingEvents does is to set _enabled to false since actual event watching is paused/stopped/hasn't begun.

This state is reached by first called BeginInit() which will call StopRaisingEvents before setting _enabled, so before we get into an IsSuspended state we will have done the subsequent cleanup necessary.

So the assertion is that when IsSuspended is true, we already did the other stuff in StopRaisingEvents and don't need to do it again.

This is ripped from the desktop code.

stephentoub · 2016-11-15T01:33:27Z

+        {
+            if (handler != null)
+            {
+                if (SynchronizingObject != null && SynchronizingObject.InvokeRequired)


This is happening in a queued work item, right? What happens if SynchronizingObject is changed from null to non-null between these checks or after these checks and before the BeginInvoke call?

The same could be said for the handler changing from non-null to null between the check and the invocation, yeah?

handler is a local

Ah yeah, we're not using the _onXHandlers directly.

The SynchronizingObject thing is potentially an issue and one that is present in desktop as well. We can't exactly catch the nullreference here because it's possible that the SynchronizingObject.BeginInvoke implementation may throw that in which case we don't want to mess with it. It also doesn't seem like we would want to copy the object or lock around it either, but I'm not too familiar with this synchronization scheme.

You can just put it in a local, e.g.

ISynchronizeInvoke syncObj = SynchronizingObject; if (syncObj != null && syncObj.InvokeRequired) { ... }

joperezr · 2016-11-15T18:41:21Z

    "System.Runtime": "4.4.0-beta-24714-01",
-    "System.ComponentModel.Primitives": "4.4.0-beta-24714-01"
+    "System.ComponentModel.Primitives": "4.4.0-beta-24714-01",
+    "System.ComponentModel.TypeConverter": "4.4.0-beta-24714-01"


4.4.0-beta-24714-01 [](start = 44, length = 19)

Just as FYI, whenever adding dependencies like in here, make sure that you rebase and update the dependency version before merging, since it is likely that they will change from the moment you sent out your review to the moment you merge.

stephentoub · 2016-11-16T01:39:43Z


        // Used for synchronization
        private bool _disposed;
+        private ISynchronizeInvoke synchronizingObject;


Nit: _synchronizingObject

Adds source and tests for the remaining missing members in FileSystemWatcher. This required that FileSystemWatcher take a dependency on System.ComponentModel.TypeConverter.

danmoseley · 2016-11-16T20:54:07Z

Not sure what needs to be done here. @mmitche is this a setup issue?

error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using the v100 build tools, please install Visual Studio 2010 build tools. Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [D:\j\workspace\windows_nt_de---4526f5ff\src\System.ServiceProcess.ServiceController\tests\System.ServiceProcess.ServiceController.TestNativeService\System.ServiceProcess.ServiceController.TestNativeService.vcxproj]

joperezr · 2016-11-16T22:54:31Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test please (bug in the build has been fixed)

ianhays added area-System.IO netstandard API labels Nov 14, 2016

ianhays added this to the 1.2.0 milestone Nov 14, 2016

dnfclas added the cla-already-signed label Nov 14, 2016

danmoseley approved these changes Nov 14, 2016

View reviewed changes

stephentoub reviewed Nov 15, 2016

View reviewed changes

karelz assigned ianhays Nov 15, 2016

joperezr reviewed Nov 15, 2016

View reviewed changes

JeremyKuhne approved these changes Nov 15, 2016

View reviewed changes

stephentoub reviewed Nov 16, 2016

View reviewed changes

stephentoub approved these changes Nov 16, 2016

View reviewed changes

Add missing FileSystemWatcher methods

7e87de2

Adds source and tests for the remaining missing members in FileSystemWatcher. This required that FileSystemWatcher take a dependency on System.ComponentModel.TypeConverter.

ianhays merged commit d8fc810 into dotnet:master Nov 17, 2016

ianhays deleted the api_fsw branch April 25, 2017 18:04

KalleOlaviNiemitalo mentioned this pull request May 8, 2021

Remove unnecessary Dispose Override from BackgroundWorker dotnet/runtime#52504

Closed

		@@ -473,7 +496,14 @@ protected void OnDeleted(FileSystemEventArgs e)
		[SuppressMessage("Microsoft.Security", "CA2109:ReviewVisibleEventHandlers", MessageId = "0#", Justification = "Changing from protected to private would be a breaking change")]
		protected void OnError(ErrorEventArgs e)

Conversation

ianhays commented Nov 14, 2016

Uh oh!

danmoseley commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joperezr commented Nov 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danmoseley commented Nov 16, 2016 •

edited

Loading