Skip to content
Open
25 changes: 24 additions & 1 deletion src/Config/ConfigFileWatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ namespace Azure.DataApiBuilder.Config;
/// <seealso cref="https://learn.microsoft.com/en-us/dotnet/api/system.io.filesystemwatcher.onchanged#remarks"/>
/// <seealso cref="https://learn.microsoft.com/en-us/dotnet/api/system.io.filesystemwatcher.notifyfilter"/>
/// <seealso cref="https://learn.microsoft.com/en-us/aspnet/core/fundamentals/change-tokens#:~:text=exponential%20back%2Doff.-,Utilities/Utilities.cs%3A,-C%23"/>
public class ConfigFileWatcher
public class ConfigFileWatcher : IDisposable
{
private bool _disposed;

/// <summary>
/// Watches a specific file for modifications and alerts
/// this class when a change is detected.
Expand Down Expand Up @@ -120,4 +122,25 @@ private void OnConfigFileChange(object sender, FileSystemEventArgs e)
Console.WriteLine("Unable to hot reload configuration file due to " + ex.Message);
}
}

/// <summary>
/// Disposes the file watcher and unsubscribes from events to release
/// file handles and prevent further file change notifications.
/// </summary>
public void Dispose()
{
if (_disposed)
{
return;
}

_disposed = true;

if (_fileWatcher is not null)
{
_fileWatcher.EnableRaisingEvents = false;
_fileWatcher.Changed -= OnConfigFileChange;
_fileWatcher.Dispose();
}
}
}
24 changes: 23 additions & 1 deletion src/Config/FileSystemRuntimeConfigLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ namespace Azure.DataApiBuilder.Config;
/// which allows for mocking of the file system in tests, providing a way to run the test
/// in isolation of other tests or the actual file system.
/// </remarks>
public class FileSystemRuntimeConfigLoader : RuntimeConfigLoader
public class FileSystemRuntimeConfigLoader : RuntimeConfigLoader, IDisposable
{
private bool _disposed;
/// <summary>
/// This stores either the default config name e.g. dab-config.json
/// or user provided config file which could be a relative file path,
Expand Down Expand Up @@ -102,6 +103,27 @@ public FileSystemRuntimeConfigLoader(
_logBuffer = logBuffer;
}

/// <summary>
/// Disposes the config file watcher to release file handles and stop
/// monitoring the config file for changes.
/// </summary>
public void Dispose()
{
if (_disposed)
{
return;
}

_disposed = true;

if (_configFileWatcher is not null)
{
_configFileWatcher.NewFileContentsDetected -= OnNewFileContentsDetected;
_configFileWatcher.Dispose();
_configFileWatcher = null;
}
}

/// <summary>
/// Get the directory name of the config file and
/// return as a string.
Expand Down
19 changes: 18 additions & 1 deletion src/Service.Tests/Configuration/ConfigurationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -710,9 +710,26 @@ type Moon {
[TestCleanup]
public void CleanupAfterEachTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

The true auto-dispose (as mentioned in PR description) of FileSystemRuntimeConfigLoader when TestServer shuts down can be done with using block. tests in ConfigurationTests.cs create TestServer without using. If the server isn't explicitly disposed, the DI container's IDisposable cleanup may never run and the file watcher could be undisposed. retry in CleanupAfterEachTest covers this as a safety net but the more correct fix would be ensuring those TestServer instances are disposed. This could be a follow-up PR if not possible in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, the 20 or so TestServer instances that don't have using won't trigger the auto-dispose, Ill follow up on this in a future PR to keep the scope of this one from growing, and the CleanupAfterEachTest should cover us in the meantime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aaronburtle,
If we dont add using in this same change, the PR's title is not serving its purpose. It is not disposing the file watchers. It is only implementing the dispose function.

I think all those FileSystemRuntimeConfigLoader should be actually disposed as well in this same PR.

{
// Retry file deletion with exponential back-off to handle cases where a
// file watcher or hot-reload process may still hold a handle on the file.
if (File.Exists(CUSTOM_CONFIG_FILENAME))
{
File.Delete(CUSTOM_CONFIG_FILENAME);
int retryCount = 0;
const int maxRetries = 3;
while (true)
{
try
{
File.Delete(CUSTOM_CONFIG_FILENAME);
break;
}
catch (IOException ex) when (retryCount < maxRetries)
{
retryCount++;
Console.WriteLine($"CleanupAfterEachTest: Retry {retryCount}/{maxRetries} deleting {CUSTOM_CONFIG_FILENAME}. {ex.Message}");
Thread.Sleep(TimeSpan.FromSeconds(Math.Pow(2, retryCount)));
}
}
}

TestHelper.UnsetAllDABEnvironmentVariables();
Expand Down