EventHandler Leak?

[blakepell]

First I wanted to say thank you for sharing. It's a really cool class. I love that it's simple but also pretty powerful.

Question:

Is there an EventHandler leak here (I think there is unless Dispose is unwiring that event)? Even though the _timer is Disposed of I think the EventHandler still needs to be unsubscribed from before that (not sure how to do that with a lambda, my solution was to make the event handler it's own function that way I could unsubscribe from it -= MyEvent before Dispose was called).

// This needs to be unsubscribed from before _timer is disposed.
_timer.Elapsed += async (sender, args) =>

[changhuixu]

@blakepell Thanks for pointing this out.

I think that the Timer instance will automatically unsubscribe events

/// <summary>
/// Disposes all the resources associated with this component.
/// </summary>
protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        lock (this)
        {
            _site?.Container?.Remove(this);
            if (_events != null)
            {
                ((EventHandler?)_events[s_eventDisposed])?.Invoke(this, EventArgs.Empty);
            }
        }
    }
}

[christianmorante]

Thanks a lot to Changhui Xu. The article and the sample implementation were really useful.

Hi Blake, you know I've been getting NullReference exceptions inside the body of the event handler, I think it must be due to setting the timer equal to null, but I don't know if that's what you mean with the event handler leak

[christianmorante]

The image capture corresponds to the error registered in the Windows Event Viewer and indicates the error in the BaseServiceWorker (based on the CronJobService)

[blakepell]

@Changhui Xu That's awesome, didn't realize it did cleaned that event up for me (I've been manually doing it for years, hehe).

@christianmorante I haven't received any weird null exception errors yet but I will keep an eye out (I have a .NET 6 site, it's in memory 24/7 and I'm using this to run some database cleanup code in the middle of the night). I shuffled some code around also though when I mistakenly thought it was leaking the event handler so mine looks like below. My first thought of what to look for is a race condition, what happens if it's running Elapsed maybe for a long running time.. e.g. the object is disposed in it's own elapsed, the DoWork takes a long time and the garbage collector comes after it in the interim because it was disposed of. I'm not sure how C# behaves in that scenario or if there's a difference in that case between the lambda in this library or how I changed it by having it as it's own function.

What value is it saying is null on your line 62 in BaseServiceWorker?

protected virtual async Task ScheduleJob(CancellationToken cancellationToken)
{
    var next = _expression.GetNextOccurrence(DateTimeOffset.Now, _timeZoneInfo);

    if (!next.HasValue)
    {
        return;
    }

    var delay = next.Value - DateTimeOffset.Now;

    // Prevent non-positive values from being passed into Timer
    if (delay.TotalMilliseconds <= 0)
    {
        await ScheduleJob(_cancellationToken);
    }

    _timer = new Timer(delay.TotalMilliseconds);
    _timer.Elapsed += Elapsed;
    _timer.Start();
}

private async void Elapsed(object? sender, System.Timers.ElapsedEventArgs e)
{
    if (_timer != null)
    {
        _timer.Elapsed -= Elapsed;
        _timer.Dispose();
        _timer = null;
    }

    if (!_cancellationToken.IsCancellationRequested)
    {
        await DoWork(_cancellationToken);
    }

    if (!_cancellationToken.IsCancellationRequested)
    {
        await ScheduleJob(_cancellationToken); // Reschedule next
    }
}

[christianmorante]

Thanks for your time @blakepell. It is just when closing the lambda function that is to say that the exception may be occurring inside or in the recursion.

Sorry for my dirty code. And of course I have severals jobs working with different scheduling