Stability fixes (#1131)

* Make the disposing of application context thread safe

* Fix the example app using get calendar events

* Make the handling of cancellation more roboust to avoid the source to be disposed before all tasks been completed

* added tests

* Fixed the throw instead

* Ignore and tracelog the operation cancelled exception in background task

* revert throwifcanceled #1

* revert throwif cancelled #2

* Add the Flush feature on BackgroundTaskTracker

* Fix the scheduler to be more robust and not call actions on disposed schedulers

* Fix the code comment using interlocked instead

* Fix warning and missing setting the disposed flag

* add test

* Added context tests

* Added even more tests

* add tests

* fix test

* remove the last of throwifcancelled

* Add one more test

* renaming

* Fix comments and from discussion

* removed timing using TaskSource

* using interlocked on more places

* removed the disposable timer

* fix last interlocked case

* forgott the test

* revert back to volatile bools

* Added more checks for disposed and throws if so

* added test for multople dispose

* fix from comment

Author: helto4real

src/AppModel/NetDaemon.AppModel.Tests/AppModelTests.cs

@@ -233,6 +233,21 @@ public async Task TestGetApplicationsLocalWithDisposable()
         app!.DisposeIsCalled.Should().BeTrue();
     }
 
+    [Fact]
+    public async Task TestApplicationCanDisposeMultipleTimesWithoutExceptions()
+    {
+        // ARRANGE
+        // ACT
+        var loadApps = await TestHelpers.GetLocalApplicationsFromYamlConfigPath("Fixtures/Local");
+
+        // CHECK
+
+        // check the application instance is init ok
+        var application = loadApps.First(n => n.Id == "LocalApps.MyAppLocalAppWithDispose");
+        await application.DisposeAsync().ConfigureAwait(false);
+        await application.DisposeAsync().ConfigureAwait(false);
+    }
+
     [Fact]
     public async Task TestGetApplicationsLocalWithAsyncDisposable()
     {

src/AppModel/NetDaemon.AppModel.Tests/Context/ApplicationContextTests.cs

@@ -0,0 +1,18 @@
+using NetDaemon.AppModel.Internal;
+using NetDaemon.AppModel.Internal.AppFactories;
+
+namespace NetDaemon.AppModel.Tests.Context;
+
+public class ApplicationContextTests
+{
+    [Fact]
+    public async Task TestApplicationContextIsDisposedMultipleTimesNotThrowsException()
+    {
+        var serviceProvider = new ServiceCollection().BuildServiceProvider();
+        var appFactory = Mock.Of<IAppFactory>();
+        var applicationContext = new ApplicationContext(serviceProvider, appFactory);
+
+        await applicationContext.DisposeAsync();
+        await applicationContext.DisposeAsync();
+    }
+}

src/AppModel/NetDaemon.AppModel/Internal/AppModelContext.cs

@@ -10,7 +10,7 @@ internal class AppModelContext : IAppModelContext
     private readonly IServiceProvider _provider;
     private readonly FocusFilter _focusFilter;
     private ILogger<AppModelContext> _logger;
-    private bool _isDisposed;
+    private volatile bool _isDisposed;
 
     public AppModelContext(IEnumerable<IAppFactoryProvider> appFactoryProviders, IServiceProvider provider, FocusFilter focusFilter, ILogger<AppModelContext> logger)
     {

src/AppModel/NetDaemon.AppModel/Internal/Context/ApplicationContext.cs

@@ -6,7 +6,7 @@ internal sealed class ApplicationContext : IAsyncDisposable
 {
     private readonly CancellationTokenSource _cancelTokenSource = new();
     private readonly IServiceScope? _serviceScope;
-    private bool _isDisposed;
+    private volatile bool _isDisposed;
 
     public ApplicationContext(IServiceProvider serviceProvider, IAppFactory appFactory)
     {
@@ -34,9 +34,7 @@ public async Task InitializeAsync()
 
     public async ValueTask DisposeAsync()
     {
-        // prevent multiple Disposes because the Service Scope will also dispose this
         if (_isDisposed) return;
-
         _isDisposed = true;
 
         if (!_cancelTokenSource.IsCancellationRequested)

src/Client/NetDaemon.HassClient.Tests/HomeAssistantClientTest/HomeAssistantConnectionTests.cs

@@ -0,0 +1,65 @@
+namespace NetDaemon.HassClient.Tests.HomeAssistantClientTest;
+
+public record FakeCommand : CommandMessage {}
+
+public class HomeAssistantConnectionTests
+{
+    private static IHomeAssistantConnection GetDefaultHomeAssistantConnection()
+    {
+        var pipeline = new TransportPipelineMock();
+        pipeline.Setup(n => n.WebSocketState).Returns(WebSocketState.Open);
+        var apiManagerMock = new Mock<IHomeAssistantApiManager>();
+        var loggerMock = new Mock<ILogger<IHomeAssistantConnection>>();
+        return new HomeAssistantConnection(loggerMock.Object, pipeline.Object, apiManagerMock.Object);
+    }
+
+    [Fact]
+    public async Task UsingDisposedConnectionWhenSendCommandShouldThrowException()
+    {
+        var homeAssistantConnection = GetDefaultHomeAssistantConnection();
+        await homeAssistantConnection!.DisposeAsync();
+
+        Func<Task> act = async () =>
+        {
+            await homeAssistantConnection!.SendCommandAsync(new FakeCommand(), CancellationToken.None);
+        };
+
+        await act.Should().ThrowAsync<ObjectDisposedException>();
+    }
+
+    [Fact]
+    public async Task UsingDisposedConnectionWhenGetApiCommandShouldThrowException()
+    {
+        var homeAssistantConnection = GetDefaultHomeAssistantConnection();
+        await homeAssistantConnection!.DisposeAsync();
+
+        Func<Task> act = async () =>
+        {
+            await homeAssistantConnection!.GetApiCallAsync<string>("test", CancellationToken.None);
+        };
+
+        await act.Should().ThrowAsync<ObjectDisposedException>();
+    }
+
+    [Fact]
+    public async Task UsingDisposedConnectionWhenPostApiShouldThrowException()
+    {
+        var homeAssistantConnection = GetDefaultHomeAssistantConnection();
+        await homeAssistantConnection!.DisposeAsync();
+
+        Func<Task> act = async () =>
+        {
+            await homeAssistantConnection!.PostApiCallAsync<string>("test", CancellationToken.None, null);
+        };
+
+        await act.Should().ThrowAsync<ObjectDisposedException>();
+    }
+
+    [Fact]
+    public async Task HomeAssistantConnectionDisposedMultipleTimesShouldNotThrow()
+    {
+        var homeAssistantConnection = GetDefaultHomeAssistantConnection();
+        await homeAssistantConnection!.DisposeAsync();
+        await homeAssistantConnection!.DisposeAsync();
+    }
+}

src/Client/NetDaemon.HassClient/Internal/HomeAssistantConnection.cs

@@ -4,6 +4,8 @@ internal class HomeAssistantConnection : IHomeAssistantConnection, IHomeAssistan
 {
     #region -- private declarations -
 
+    private volatile bool _isDisposed;
+
     private readonly ILogger<IHomeAssistantConnection> _logger;
     private readonly IWebSocketClientTransportPipeline _transportPipeline;
     private readonly IHomeAssistantApiManager _apiManager;
@@ -132,6 +134,9 @@ public async Task SendCommandAsync<T>(T command, CancellationToken cancelToken)
 
     public async ValueTask DisposeAsync()
     {
+        if (_isDisposed) return;
+        _isDisposed = true;
+
         try
         {
             await CloseAsync().ConfigureAwait(false);
@@ -159,18 +164,22 @@ await Task.WhenAny(
 
     public Task<T?> GetApiCallAsync<T>(string apiPath, CancellationToken cancelToken)
     {
+        ObjectDisposedException.ThrowIf(_isDisposed, nameof(HomeAssistantConnection));
         return _apiManager.GetApiCallAsync<T>(apiPath, cancelToken);
     }
 
     public Task<T?> PostApiCallAsync<T>(string apiPath, CancellationToken cancelToken, object? data = null)
     {
+        ObjectDisposedException.ThrowIf(_isDisposed, nameof(HomeAssistantConnection));
         return _apiManager.PostApiCallAsync<T>(apiPath, cancelToken, data);
     }
 
     public IObservable<HassMessage> OnHassMessage => _hassMessageSubject;
 
     private async Task<Task<HassMessage>> SendCommandAsyncInternal<T>(T command, CancellationToken cancelToken) where T : CommandMessage
     {
+        ObjectDisposedException.ThrowIf(_isDisposed, nameof(HomeAssistantConnection));
+
         // The semaphore can fail to be taken in rare cases so we need
         // to keep this out of the try/finally block so it will not be released
         await _messageIdSemaphore.WaitAsync(cancelToken).ConfigureAwait(false);

src/Extensions/NetDaemon.Extensions.MqttEntityManager/MessageSubscriber.cs

@@ -21,7 +21,7 @@ namespace NetDaemon.Extensions.MqttEntityManager;
 internal class MessageSubscriber : IMessageSubscriber, IDisposable
 {
     private readonly SemaphoreSlim _subscriptionSetupLock = new SemaphoreSlim(1);
-    private bool _isDisposed;
+    private volatile bool _isDisposed;
     private bool _subscriptionIsSetup;
     private readonly IAssuredMqttConnection _assuredMqttConnection;
     private readonly ILogger<MessageSubscriber> _logger;
@@ -121,17 +121,16 @@ private Task OnMessageReceivedAsync(MqttApplicationMessageReceivedEventArgs msg)
 
     public void Dispose()
     {
-        if (!_isDisposed)
-        {
-            _isDisposed = true;
-            foreach (var observer in _subscribers)
-            {
-                _logger.LogTrace("Disposing {Topic} subscription", observer.Key);
-                observer.Value.Value.OnCompleted();
-                observer.Value.Value.Dispose();
-            }
+        if (_isDisposed) return;
+        _isDisposed = true;
 
-            _subscriptionSetupLock.Dispose();
+        foreach (var observer in _subscribers)
+        {
+            _logger.LogTrace("Disposing {Topic} subscription", observer.Key);
+            observer.Value.Value.OnCompleted();
+            observer.Value.Value.Dispose();
         }
+
+        _subscriptionSetupLock.Dispose();
     }
-}
+}

src/Extensions/NetDaemon.Extensions.Scheduling.Tests/Scheduling/DisposableSchedulerTest.cs

@@ -35,6 +35,47 @@ public void _SchedulePeriodicStopsAfterDisposeOfDisposableScheduler()
     }
 
 
+    [Fact]
+    public void PeriodicSchedulerShouldNotCallActionIfItIsDisposedDuringSchedule()
+    {
+        var (inner, disposableScheduler) = CreateScheduler();
+
+        int called = 0;
+        using var _ = disposableScheduler.SchedulePeriodic(TimeSpan.FromMinutes(1), () => called++);
+
+        // Dispose before the time moves forward and trigger a schedule
+        disposableScheduler.Dispose();
+
+        inner.AdvanceBy(TimeSpan.FromMinutes(1).Ticks);
+        called.Should().Be(0);
+    }
+
+    [Fact]
+    public void PeriodicSchedulerShouldNotCallActionIfItIsDisposedBeforeScheduled()
+    {
+        var (inner, disposableScheduler) = CreateScheduler();
+
+        int called = 0;
+        // Dispose before the we call schedule
+        disposableScheduler.Dispose();
+        using var _ = disposableScheduler.SchedulePeriodic(TimeSpan.FromMinutes(1), () => called++);
+        inner.AdvanceBy(TimeSpan.FromMinutes(1).Ticks);
+        called.Should().Be(0);
+    }
+
+    [Fact]
+    public void SchedulerShouldNotCallActionIfItIsDisposedBeforeScheduled()
+    {
+        var (inner, disposableScheduler) = CreateScheduler();
+
+        int called = 0;
+        // Dispose before the we call schedule
+        disposableScheduler.Dispose();
+        using var _ = disposableScheduler.Schedule(() => called++);
+        inner.AdvanceBy(TimeSpan.FromMinutes(1).Ticks);
+        called.Should().Be(0);
+    }
+
     [Fact]
     public void SchedulePeriodicStopsAfterDisposeOfDisposableScheduler()
     {