Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds debouncing FileSystemWatcher events #129

Merged
merged 5 commits into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/Plugins/Internal/Debouncer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
using System;
using System.Threading;
using System.Threading.Tasks;

namespace McMaster.NETCore.Plugins.Internal
{
internal class Debouncer : IDisposable
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some unit tests for this to verify its behavior (and prevent regressions in the future)?

Copy link
Contributor Author

@thoemmi thoemmi Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed some unit tests. Do you think they're sufficient?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you!

{
private readonly CancellationTokenSource _cts = new CancellationTokenSource();
private readonly TimeSpan _waitTime;
private int _counter;

public Debouncer(TimeSpan waitTime)
{
_waitTime = waitTime;
}

public void Execute(Action action)
{
var current = Interlocked.Increment(ref _counter);

Task.Delay(_waitTime).ContinueWith(task =>
{
// Is this the last task that was queued?
if (current == _counter && !_cts.IsCancellationRequested)
{
action();
}

task.Dispose();
}, _cts.Token);
}

public void Dispose()
{
_cts.Cancel();
}
}
}
6 changes: 6 additions & 0 deletions src/Plugins/PluginConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ public bool IsUnloadable
/// Use the event <see cref="PluginLoader.Reloaded" /> to be notified of changes.
/// </summary>
public bool EnableHotReload { get; set; }

/// <summary>
/// Specifies the delay to reload a plugin, after file changes have been detected.
/// Default value is 200 milliseconds.
/// </summary>
public TimeSpan ReloadDelay { get; set; } = TimeSpan.FromMilliseconds(200);
#endif
}
}
9 changes: 7 additions & 2 deletions src/Plugins/PluginLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.IO;
using System.Reflection;
using System.Runtime.Loader;
using McMaster.NETCore.Plugins.Internal;
using McMaster.NETCore.Plugins.Loader;

namespace McMaster.NETCore.Plugins
Expand Down Expand Up @@ -153,6 +154,7 @@ public static PluginLoader CreateFromAssemblyFile(string assemblyFile, Action<Pl

#if FEATURE_UNLOAD
private FileSystemWatcher? _fileWatcher;
private Debouncer? _debouncer;
#endif

/// <summary>
Expand Down Expand Up @@ -223,13 +225,14 @@ This is a very simple implementation.
Some improvements that could be made in the future:

* Watch all directories which contain assemblies that could be loaded
* Debounce changes. When files are written in-place, there can be multiple events within a few milliseconds.
* Support a polling file watcher.
* Handle delete/recreate better.

If you're interested in making improvements, feel free to send a pull request.
*/

_debouncer = new Debouncer(_config.ReloadDelay);

_fileWatcher = new FileSystemWatcher();
_fileWatcher.Path = Path.GetDirectoryName(_config.MainAssemblyPath);
_fileWatcher.Changed += OnFileChanged;
Expand All @@ -242,7 +245,7 @@ private void OnFileChanged(object source, FileSystemEventArgs e)
{
if (!_disposed)
{
Reload();
_debouncer?.Execute(Reload);
}
}
#endif
Expand Down Expand Up @@ -322,6 +325,8 @@ public void Dispose()
_fileWatcher.Dispose();
}

_debouncer?.Dispose();

if (_context.IsCollectible)
{
_context.Unload();
Expand Down
4 changes: 3 additions & 1 deletion src/Plugins/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
McMaster.NETCore.Plugins.Loader.AssemblyLoadContextBuilder.ShadowCopyNativeLibraries() -> McMaster.NETCore.Plugins.Loader.AssemblyLoadContextBuilder
McMaster.NETCore.Plugins.Loader.AssemblyLoadContextBuilder.ShadowCopyNativeLibraries() -> McMaster.NETCore.Plugins.Loader.AssemblyLoadContextBuilder
McMaster.NETCore.Plugins.PluginConfig.ReloadDelay.get -> System.TimeSpan
McMaster.NETCore.Plugins.PluginConfig.ReloadDelay.set -> void
57 changes: 57 additions & 0 deletions test/Plugins.Tests/DebouncerTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
using System;
using System.Threading.Tasks;
using McMaster.NETCore.Plugins.Internal;
using Xunit;

namespace McMaster.NETCore.Plugins.Tests
{
public class DebouncerTests
{
[Fact]
public async Task InvocationIsDelayed()
{
var executionCounter = 0;

var debouncer = new Debouncer(TimeSpan.FromSeconds(.1));
debouncer.Execute(() => executionCounter++);

Assert.Equal(0, executionCounter);

await Task.Delay(TimeSpan.FromSeconds(.5));

Assert.Equal(1, executionCounter);
}

[Fact]
public async Task ActionsAreDebounced()
{
var executionCounter = 0;

var debouncer = new Debouncer(TimeSpan.FromSeconds(.1));
debouncer.Execute(() => executionCounter++);
debouncer.Execute(() => executionCounter++);
debouncer.Execute(() => executionCounter++);

await Task.Delay(TimeSpan.FromSeconds(.5));

Assert.Equal(1, executionCounter);
}

[Fact]
public async Task OnlyLastActionIsInvoked()
{
string? invokedAction = null;

var debouncer = new Debouncer(TimeSpan.FromSeconds(.1));
foreach (var action in new[]{"a", "b", "c"})
{
debouncer.Execute(() => invokedAction = action);
}

await Task.Delay(TimeSpan.FromSeconds(.5));

Assert.NotNull(invokedAction);
Assert.Equal("c", invokedAction);
}
}
}
15 changes: 12 additions & 3 deletions test/TestProjects/SqlClientApp/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,19 @@ public static void Main(string[] args)

public static bool Run()
{
using (var client = new SqlConnection(@"Data Source=(localdb)\mssqllocaldb;Integrated Security=True"))
try
{
client.Open();
return !string.IsNullOrEmpty(client.ServerVersion);
using (var client = new SqlConnection(@"Data Source=(localdb)\mssqllocaldb;Integrated Security=True"))
{
client.Open();
return !string.IsNullOrEmpty(client.ServerVersion);
}
}
catch (SqlException ex) when (ex.Number == -2) // -2 means SQL timeout
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the SqlException start happening with this change? Was it consistent, or just a flaky test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno, the build pipleline failed: https://github.com/natemcmaster/DotNetCorePlugins/runs/486084609
I'm not sure if it has been a transient error.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen it before. This test isn't using reloading though, so it's probably not related to this change.

{
// When running the test in Azure DevOps build pipeline, we'll get a SqlException with "Connection Timeout Expired".
// We can ignore this safely in unit tests.
return true;
}
}
}
Expand Down