События в многопоточной среде

Question

Я пытаюсь построить систему, с помощью которой пользователи могут создавать небольшую тестовую программу, не зная, как писать код. Для этого я спроектировал систему таким образом, чтобы существовала процедура, которая может содержать другие процедуры или этапы. Шаги могут содержать команды. Процедура содержит лог c, что происходит в каком порядке. Шаги содержат информацию, к какому шагу они подключены далее.

Шаги и команды вызываются Execute и вызывают OnDone, если они завершены, что может произойти непосредственно (например, IncreaseCommand) или после определенный период времени (WaitCommand или любая другая команда, связывающаяся с подключенным оборудованием; обе находятся в другом потоке). Кроме того, они могут быть остановлены тайм-аутом или пользователем.

Пока нет тайм-аута, все работает нормально. Если есть тайм-аут, я попытался сделать поток кода безопасным путем блокировки. Кроме того, существуют эти ловушки, когда тайм-аут останавливает команду (например, WaitCommand), которая завершает свою работу в тот же момент. Таким образом, есть один поток, проходящий путь от процедуры через шаг к команде, сигнализирующий об остановке, и другой поток, работающий от команды через шаг к выполненной сигнализации процедуры.

Я добавил несколько фрагментов кода, которые я Я удалил большую часть кода утилизации и других внутренних вещей, которые, похоже, не связаны с этой проблемой.

public sealed class Procedure : IStep, IStoppable
{
    public event EventHandler Done;
    public event EventHandler Stopped;
    public event EventHandler TimedOut;
    public void Run()
    {
        if (!IsRunning)
        {
            CheckStartTimer();
            Start(First);
        }
    }
    private void CheckStartTimer()
    {
        isTimerUnlinked = false;
        timer.Elapsed += OnTimedOut;
        timer.IntervalInMilliseconds = (int)Timeout.TotalMilliseconds;
        timer.Start();
    }
    private void OnTimedOut(object sender, EventArgs e)
    {
        if (isTimerUnlinked)
            return;
        stopFromTimeout = true;
        Stop();
    }
    private void Start(IStep step)
    {
        isStopped = false;
        isStopping = false;
        Active = step;
        LinkActive();
        active.Run();
    }
    private void LinkActive()
    {
        active.Done += OnActiveFinished;
        if (active is Procedure proc)
            proc.TimedOut += OnActiveFinished;
    }
    private void OnActiveFinished(object sender, EventArgs e)
    {
        UnlinkActive();
        lock (myLock)
        {
            if (isStopped)
                return;
            if (stopFromTimeout)
            {
                OnStopped();
                return;
            }
        }
        var successor = active.ActiveSuccessor;
        if (successor == null)
            OnDone();
        else if (isStopping || timeoutPending || stopFromTimeout)
            OnStopped();
        else
            Start(successor);
    }
    public void Stop()
    {
        if (isStopping)
            return;
        isStopping = true;
        StopTimer();
        if (active is IStoppable stoppable)
        {
            stoppable.Stopped += stoppable_Stopped;
            stoppable.Stop();
        }
        else
            OnStopped();
    }
    private void stoppable_Stopped(object sender, EventArgs e)
    {
        var stoppable = sender as IStoppable;
        stoppable.Stopped -= stoppable_Stopped;
        OnStopped();
    }
    private void OnStopped()
    {
        isStopping = false;
        lock (myLock)
        {
            isStopped = true;
        }
        UnlinkActive();
        lock (myLock)
        {
            Active = null;
        }
        if (stopFromTimeout || timeoutPending)
        {
            stopFromTimeout = false;
            timeoutPending = false;
            CleanUp();
            TimedOut?.Invoke(this, EventArgs.Empty);
        }
        else
            Stopped?.Invoke(this, EventArgs.Empty);
    }
    private void UnlinkActive()
    {
        if (stopFromTimeout && !isStopped)
            return;
        lock (myLock)
        {
            if (active == null)
                return;
            active.Done -= OnActiveFinished;
            var step = active as IStep;
            if (step is Procedure proc)
                proc.TimedOut -= OnActiveFinished;
        }
    }
    private void OnDone()
    {
        CleanUp();
        Done?.Invoke(this, EventArgs.Empty);
    }
    private void CleanUp()
    {
        Reset();
        SetActiveSuccessor();
    }
    private void Reset()
    {
        Active = null;
        stopFromTimeout = false;
        timeoutPending = false;
        StopTimer();
    }
    private void StopTimer()
    {
        if (timer == null)
            return;
        isTimerUnlinked = true;
        timer.Elapsed -= OnTimedOut;
        timer.Stop();
    }
    private void SetActiveSuccessor()
    {
        ActiveSuccessor = links[(int)Successor.Simple_If];
    }
}

internal sealed class CommandStep : IStep, IStoppable
{
    public event EventHandler Done;
    public event EventHandler Started;
    public event EventHandler Stopped;
    public CommandStep(ICommand command)
    {
        this.command = command;
    }
    public void Run()
    {
        lock (myLock)
        {
            stopCalled = false;
            if (cookie != null && !cookie.Signalled)
                throw new InvalidOperationException(ToString() + " is already active.");
            cookie = new CommandStepCookie();
        }
        command.Done += OnExit;
        unlinked = false;
        if (stopCalled)
            return;
        command.Execute();
    }
    public void Stop()
    {
        stopCalled = true;
        if (command is IStoppable stoppable)
            stoppable.Stop();
        else
            OnExit(null, new CommandEventArgs(ExitReason.Stopped));
    }
    private void OnExit(object sender, CommandEventArgs e)
    {
        (sender as ICommand).Done -= OnExit;
        lock (myLock)
        {
            if (cookie.Signalled)
                return;
            cookie.ExitReason = stopCalled ? ExitReason.Stopped : e.ExitReason;
            switch (cookie.ExitReason)
            {
                case ExitReason.Done:
                default:
                    if (unlinked)
                        return;
                    Unlink();
                    ActiveSuccessor = links[(int)Successor.Simple_If];
                    break;
                case ExitReason.Stopped:
                    Unlink();
                    break;
                case ExitReason.Error:
                    throw new NotImplementedException();
            }
            cookie.Signalled = true;
        }
        if (cookie.ExitReason.HasValue)
        {
            active = false;
            if (cookie.ExitReason == ExitReason.Done)
                Done?.Invoke(this, EventArgs.Empty);
            else if (cookie.ExitReason == ExitReason.Stopped)
                stopCalled = false;
                Stopped?.Invoke(this, EventArgs.Empty);
        }
    }
    private void Unlink()
    {
        if (command != null)
            command.Done -= OnExit;
        unlinked = true;
    }
}

internal sealed class WaitCommand : ICommand, IStoppable
{
    public event EventHandler<CommandEventArgs> Done;
    public event EventHandler Stopped;
    internal WaitCommand(ITimer timer)
    {
        this.timer = timer;
        timer.AutoRestart = false;
        TimeSpan = TimeSpan.FromMinutes(1);
    }
    public void Execute()
    {
        lock (myLock)
        {
            cookie = new WaitCommandCookie(
                e => Done?.Invoke(this, new CommandEventArgs(e)));
            timer.IntervalInMilliseconds = (int)TimeSpan.TotalMilliseconds;
            timer.Elapsed += OnElapsed;
        }
        timer.Start();
    }
    private void OnElapsed(object sender, EventArgs e)
    {
        OnExit(ExitReason.Done);
    }
    public void Stop()
    {
        if (cookie == null)
        {
            Done?.Invoke(this, new CommandEventArgs(ExitReason.Stopped));
            return;
        }
        cookie.Stopping = true;
        lock (myLock)
        {
            StopTimer();
        }
        OnExit(ExitReason.Stopped);
    }
    private void OnExit(ExitReason exitReason)
    {
        if (cookie == null)
            return;
        lock (myLock)
        {
            if (cookie.Signalled)
                return;
            Unlink();
            if (cookie.Stopping && exitReason != ExitReason.Stopped)
                return;
            cookie.Stopping = false;
        }
        cookie.Signal(exitReason);
        cookie = null;
    }
    private void StopTimer()
    {
        Unlink();
        timer.Stop();
    }
    private void Unlink()
    {
        timer.Elapsed -= OnElapsed;
    }
}

В некоторых местах я проверял, нет ли остановки выполняется и пытается перехватить выполненное, чтобы оно не выполнялось после остановки и не вызывало каких-либо проблем. Этот способ не кажется полностью водонепроницаемым, хотя кажется, что он работает на данный момент. Есть ли способ обеспечить такой вид безопасности по конструкции? Может быть, я делаю это совершенно неправильно?

Answer 1

Ваше общее состояние не защищено от одновременного доступа. Например, возьмем поле isStopped:

private void OnStopped()
{
    isStopping = false;
    lock (myLock)
    {
        isStopped = true;
    }
    //...

private void Start(IStep step)
{
    isStopped = false;
    //...

Во-первых, оно защищено, во-вторых, нет. Вы можете либо защитить его везде, либо нигде. Там нет никакого среднего. Частичная его защита так же хороша, как и полная.

В качестве примечания, вызывать обработчики событий, удерживая блокировку, не рекомендуется. Обработчик событий может содержать долго выполняющийся код или вызывать произвольный код, который, возможно, защищен другими блокировками, открывая возможность для взаимоблокировок. Общий совет по блокировке - освободить его как можно быстрее. Чем дольше вы удерживаете блокировку, тем больше конфликтов вы создаете между потоками. Так, например, в методе OnActiveFinished:

lock (myLock)
{
    if (isStopped)
        return;
    if (stopFromTimeout)
    {
        OnStopped();
        return;
    }
}

Вы вызываете OnStopped, удерживая блокировку. А внутри OnStopped вы вызываете обработчики:

Stopped?.Invoke(this, EventArgs.Empty);

Правильный способ - вызвать OnStopped после снятия блокировки. Используйте локальную переменную для хранения информации о том, вызывать ее или нет:

var localInvokeOnStopped = false;
lock (myLock)
{
    if (isStopped)
        return;
    if (stopFromTimeout)
    {
        localInvokeOnStopped = true;
    }
}
if (localInvokeOnStopped)
{
    OnStopped();
    return;
}

Последний совет, избегайте рекурсивной блокировки одной и той же блокировки. Оператор lock не будет жаловаться, если вы это сделаете (поскольку базовый класс Monitor допускает повторный вход), но это делает вашу программу менее понятной и поддерживаемой.