11
votes
\$\begingroup\$

I have this piece of code I would like some help with. I'm running a lengthy process that I want to interrupt at any time. The thing is, as you can see in the code, I have if(stop) everywhere, also in the methods inside the for (since they have for loops also).

stop is a member variable of the class, and the Stop() method is called from the UI from a different thread.

I don't think making it multithreading would be a good option since would be more complex to keep state and to communicate between threads.

Can you think of a better way to solve this, making the code cleaner?

public void Start()
{
    for(int i = 0; i < Length; i++)
    {
        if(stop)
            return;

        LengthyMethodA();

        if(stop)
            return;

        LengthyMethodB();

        if(stop)
            return;

        LengthyMethodC();
    }
}

public void Stop()
{
    stop = true;
}
\$\endgroup\$
1
8
votes
\$\begingroup\$

You could always set up a list of Actions that need to be worked on, and then loop through them checking for Stop each time.

Something like :

List<Action> actions = new List<Action> {
  () => LengthyMethodA(),
  () => LengthyMethodB(),
  () => LengthyMethodC() };

  for(int i = 0; i < Length; i++)
  {    
    foreach (Action action in actions)
    {
       action();
       if(stop)
            return;
    }  
  }

This may or may not be better. It depends on the actual code. The actions list would represent an ordered set of transactional units of code - each unit must be completed, but the whole set can be broken apart and cancelled if necessary.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ The lambdas seem redundant since the called methods (appear to) have the same signature as the delegate. +1 because the rest of it looks good though :) \$\endgroup\$ – Brian Reichle Feb 22 '11 at 7:54
4
votes
\$\begingroup\$

You say that Start is running in a thread other than the UI thread and that the Stop method is called from the UI thread.

That being the case, why not just call Abort on the thread Start is running in? This throws a ThreadAbortException in the Start thread and causes your code to fall through as you intend.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ Microsoft's Managed Threading Best Practices say to avoid calling Thread.Abort on another threads. \$\endgroup\$ – mjcopple Feb 21 '11 at 22:21
  • \$\begingroup\$ Yes, it does, under "Consider the following guidelines when using multiple threads". But if killing the thread without needing to know where it has got to is your goal, which it seems to be here, then you consider it and disregard it \$\endgroup\$ – pdr Feb 22 '11 at 19:50
  • 1
    \$\begingroup\$ +1 It's really a good idea, I didn't even thought about it, I think I was too inside my code, couldn't see it from the outside (the UI :D), anyway I'm accepting mongus answer because it looks like a better practice for broader use cases. \$\endgroup\$ – rodrigoq Feb 22 '11 at 21:37
  • \$\begingroup\$ @rodrigoq - Agree. Both valid solutions, depending on the problem, and Mongo's is better in more cases \$\endgroup\$ – pdr Feb 22 '11 at 22:28
2
votes
\$\begingroup\$

Any answer given will be completely conditional. Can you be sure that stopping after step B will not have dire consequences related to the fact that step A was completed successfully? I recommend you break each section into work that must be completed with a chance to terminate before entering. Then I recommend you switch to AutoResetEvent which is thread safe. Unfortunately this really won't lead to less code but it will make your code more robust.

private readonly AutoResetEvent _stopRunning = new AutoResetEvent(false);

public void Start()
{
    for(int i = 0; i < Length; i++)
    {    
        if (_stopRunning.WaitOne(1)) return;
        WorkThatMustRunToCompletion();
    }
}

public void Stop()
{
    _stopRunning.Set();    
}
\$\endgroup\$
1
  • \$\begingroup\$ If you mark the bool as volatile then it would still be perfectly thread safe (I think even without volatile, the most it would suffer from is a delay in realising the flag had changed). If you do however use a WaitHandle like this, then I suggest passing 0 into WaitOne() instead of 1. This will avoid the cost of a block and context switch when not told to stop. \$\endgroup\$ – Brian Reichle Feb 22 '11 at 7:50
0
votes
\$\begingroup\$

Looks like so long as you aren't calling more than these three methods, what you have is better for clarity's sake. And the cost of the conditionals is pretty cheap as far as compilation goes.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.