I'm trying to build a system via which users can build a little test program without knowing how to code. For that purpose I designed the system that way, that there is a procedure, which can contain other procedures or steps. The steps can contain commands. The procedure contains the logic what happens in which order. The steps contain the information to which step they are connected next.
The steps and commands are called by Execute and invoke OnDone if they are finished which may happen directly (eg IncreaseCommand) or after a certain period of time (WaitCommand or any other command communicating with the connected hardware; both are on a different thread). Additionally they can be stopped by a timeout or by user.
As long as there is no timeout everything works fine. If there is a timeout I tried to make the code thread safe by locking. Additionally there are these pitfalls when the timeout stops a command (eg WaitCommand) which is finishing its work in the same moment. So there is one thread working its way from the procedure via the step to the command, signalling stop and another thread working from the command via the step to the procedure signalling done.
I added some code snippets, which I've stripped of most of the dispose code and other internal stuff, which seems not to be related to the issue.
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;
}
}
I've been testing at certain places whether a stop is ongoing and trying to intercept the done, so that it is not executed after the stop and cause any trouble. This way doesn't seem to be fully waterproof, althoug it seems to work for the moment. Is there a way to provide this kind of safety by design? Am I maybe doing this completely wrong?
Your shared state is not protected from concurrent access consistently. For example take the isStopped field:
private void OnStopped()
{
isStopping = false;
lock (myLock)
{
isStopped = true;
}
//...
private void Start(IStep step)
{
isStopped = false;
//...
In the first place it's protected, in the second it's not. You can either choose to protect it everywhere, or nowhere. There is no middle ground. Protecting it partially is as good as not protecting it at all.
As a side note, invoking event handlers while holding a lock is not recommended. An event handler could contain long running code, or call arbitrary code that is possibly protected by other locks, opening the possibility for deadlocks. The general advice about locking is to release it as quickly as possible. The longer you hold a lock, the more contention you create between threads. So for example in the method OnActiveFinished:
lock (myLock)
{
if (isStopped)
return;
if (stopFromTimeout)
{
OnStopped();
return;
}
}
You call the OnStopped while holding the lock. And inside the OnStopped you invoke the handlers:
Stopped?.Invoke(this, EventArgs.Empty);
The correct way is to call the OnStopped after releasing the lock. Use a local variable for storing the info about calling it or not:
var localInvokeOnStopped = false;
lock (myLock)
{
if (isStopped)
return;
if (stopFromTimeout)
{
localInvokeOnStopped = true;
}
}
if (localInvokeOnStopped)
{
OnStopped();
return;
}
Last advice, avoid locking recursively on the same lock. The lock statement will not complain if you do it (because the underlying Monitor class allows reentrancy), but it makes your program less understandable and maintainable.