Skip to content

Fix multi dispose scheduler and timer #799

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Fix multi dispose scheduler and timer #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d88dad
Check if already disposed and skip if so
Rick-van-Dam Dec 6, 2022
86de16b
Add unit test to check dispose functionality
Rick-van-Dam Dec 6, 2022
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
9 changes: 9 additions & 0 deletions src/Extensions/NetDaemon.Extensions.Scheduling.Tests/Scheduling/DisposableSchedulerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ public void SchedulePeriodicStopsAfterDisposeOfSubscriber()
inner.AdvanceBy(TimeSpan.FromMinutes(1).Ticks);
called.Should().Be(3);
}

[Fact]
public void DisposeTwice_NoException()
{
var (_, disposableScheduler) = CreateScheduler();

disposableScheduler.Dispose();
disposableScheduler.Dispose();
}

private (TestScheduler inner, DisposableScheduler disposableScheduler) CreateScheduler()
{
Expand Down
17 changes: 17 additions & 0 deletions src/Extensions/NetDaemon.Extensions.Scheduling.Tests/Scheduling/DisposableTimerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using System.Threading;
using NetDaemon.Extensions.Scheduler;
using Xunit;

namespace NetDaemon.Extensions.Scheduling.Tests;

public class DisposableTimerTest
{
[Fact]
Copy link
Contributor

@FrankBakkerNl FrankBakkerNl Dec 6, 2022

Choose a reason for hiding this comment

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

Suggestion:
Because this is really a helper class that should be internal it does not seem to deserve its own test class. You could move this test to SchedulerExtensionTest.SchedulePeriodicStopsAfterDisposeOfSubscriber() and dispose the result of disposable.RunEvery there twice

public void DisposeTwice_NoException()
{
var timer = new DisposableTimer(CancellationToken.None);

timer.Dispose();
timer.Dispose();
}
}
10 changes: 6 additions & 4 deletions src/Extensions/NetDaemon.Extensions.Scheduling/DisposableScheduler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ public IDisposable Schedule<TState>(TState state, DateTimeOffset dueTime, Func<I
/// <inheritdoc />
public void Dispose()
{
_isDisposed = true;

_cancellationTokenSource.Cancel();
_cancellationTokenSource.Dispose();
if (!_isDisposed)
{
_cancellationTokenSource.Cancel();
_cancellationTokenSource.Dispose();
_isDisposed = true;
}
}
}
11 changes: 8 additions & 3 deletions src/Extensions/NetDaemon.Extensions.Scheduling/DisposableTimer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ public sealed class DisposableTimer : IDisposable
{
Copy link
Contributor

@FrankBakkerNl FrankBakkerNl Dec 6, 2022

Choose a reason for hiding this comment

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

I think this class should have been internal actually

private readonly CancellationTokenSource _combinedToken;
private readonly CancellationTokenSource _internalToken;
private bool _disposed;

/// <summary>
/// Constructor
/// </summary>
Expand All @@ -29,8 +31,11 @@ public DisposableTimer(CancellationToken token)
/// </summary>
public void Dispose()
{
_internalToken.Cancel();
_combinedToken.Dispose();
_internalToken.Dispose();
if (!_disposed){
_internalToken.Cancel();
_combinedToken.Dispose();
_internalToken.Dispose();
_disposed = true;
}
}
}