概要

System.Timers.Timer の Elapsed イベント登録はスレッドセーフではないので適切にロックをする必要がある。

経緯

System.Timers.Timer を使用して、定期的に処理を実行させつつ処理の内容を変えるために Elapsed イベントにメソッドを指したり抜いたりしていたところ、たまに抜かれたはずの処理が走っている現象に出くわした。
まぁタイマーの使い方としてはあまり正しくない気もしなくはないが。

再現コード

class TimerRemoveFail
{
    public TimerRemoveFail()
    {
        var timer = new Timer { AutoReset = false, Interval = 1 };
        var tasks = new List<Task>();

        foreach (var _ in Enumerable.Range(0, 100))
        {
            tasks.Add(Task.Run(() =>
            {
                timer.Elapsed += OnElapsed;
                timer.Elapsed -= OnElapsed;
            }));
        }

        Task.WaitAll(tasks.ToArray());

        Console.WriteLine("Start");
        timer.Start();
        Console.ReadLine();
    }

    void OnElapsed(object _, EventArgs __)
    {
        Console.WriteLine("Not Removed!!!");
    }
}

実行結果

Start
Not Removed!!!
Not Removed!!!
Not Removed!!!
Not Removed!!!

.NET6 でも発生する。

対処方法

Elapsed に +=,-= しているところを適切に lock ステートメントでくくってしまえば良い。

原因

        public event ElapsedEventHandler Elapsed {
            add {
                onIntervalElapsed += value;
            }
            remove {
                onIntervalElapsed -= value;
            }
        }

add remove メソッドを書かずに自動実装イベントに任せたときでも Interlocked を使用してマルチスレッドに配慮されているのにどうして。。。
下記記事が参考になる。
https://ufcpp.net/study/csharp/sp_event.html#auto-event

というか

上記を見ると、Tipに

System.Timers.Timer (this topic): fires an event at regular intervals. The class is intended for use as a server-based or service component in a multithreaded environment; it has no user interface and is not visible at runtime.
(System.Timers.Timer (このトピック): 一定時間ごとにイベントを発生させます。このクラスは、マルチスレッド環境におけるサーバベースまたはサービスコンポーネントとして使用することを意図しています。）

って書いてあるのに、Noteに

The event-handling method might run on one thread at the same time that another thread calls the Stop method or sets the Enabled property to false. This might result in the Elapsed event being raised after the timer is stopped.
(イベント処理メソッドとは別のスレッドがStopメソッドを呼び出したり、Enabledプロパティをfalseに設定したりすると、タイマーを停止した後に Elapsed イベントが発生する可能性があります。)

ってあったりして、あまりマルチスレッド向きとは言えない気がするなぁ。