CancelRelayout does not stop background threads about graphx HOT 8 CLOSED

Hi, i wonder wouldn't it be better idea to abort whole thread upon cancellation request. That approach will spare too much cancellation checks in the logic.

from graphx.

Aborting a thread is unsafe. (http://stackoverflow.com/a/5299124) It might be quick and easy, but it is something you want to do only in exceptional cases.

from graphx.

Without having looked at the code in question, I'd suggest using a ManualResetEventSlim as it's rather fast, and does not require slow kernel objects.

from graphx.

I think I'll have a go at it this evening, and do some measurements on it. Don't want to slow down the layout code needlessly!

from graphx.

I've almost forgot about this task, that is also an option to implement :)

from graphx.

I've been looking at a solution for this issue, but I need to confirm with you first, so that I don't step onto future plans of yours.

If I understand the code correctly, all algorithm implementations are fully sequential (no method spawns a new thread/task/worker). The launch of each algorithm however (GraphArea<TVertex, TEdge, TGraph>._relayoutGraphMain(bool, bool)) may be done concurrently in a background worker if requested.

AlgorithmBase seems to have been designed to support cancellation (see AlgorithmBase.Abort()). Despite the support for cancellation, I can only find one concrete implementation FRLayoutAlgorithm that supports cancellation. Is this design part of a long term goal?

I've been toying with the idea of using a volatile bool wrapped inside an object. The object, implements the fictitious interface:

interface IAlgorithmControl {
     void ThrowIfAborted { get; }
}

A concrete implementation of this interface would look like:

class AlgorithmControl : IAlgorithmControl {
   private volatile bool _abortRequested;

   public void RequestAbort() {
      _abortRequested = true;
   }

   public void ThrowIfAborted() {
      if(_abortRequested)
         throw new OperationCanceledException();
   }
}

An instance of this concrete class is then passed to each Compute() method as an argument. Each implementation may periodically call ThrowIfAborted(). Typically it is instantiated in GraphArea<TVertex, TEdge, TGraph>._relayoutGraphMain(bool, bool) like this:

...
Worker.DoWork += ((sender, e) =>
{
   try
   {
      _algorithmControl = new AlgorithmControl();
      _relayoutGraph(_algorithmControl);
   }
   catch (OperationCanceledException)
   {
      // Possibly do some clean up
   }
};
...

In CancelRelayout():

public void CancelRelayout()
{
   if(_algorithmControl != null)
      _algorithmControl.RequestAbort();
}

We also need to join the background worker upon termination, because there are race conditions lurking here. Even though a background worker has been requested to terminate, it will still keep on modifying the graph, and odds are that another worker is executing at the same time. The actual changes to the graph is serialized by the UI thread, but that will only make the concurrent changes coarse.

I think this would be the most efficient way to cancel a background layout worker. Why not use the cancellation mechanism in place with the background worker you might ask? Well, I did consider it, but I changed my mind when I looked at the implementation of the cancellation (http://referencesource.microsoft.com/#System/compmod/system/componentmodel/BackgroundWorker.cs). The implementation uses a non-volatile bool as a flag for determining whether a cancellation is in progress or not. I'm not sure, but it does not seem right as the compiler and JIT may generate code that prevents the background worker to detect a change from false to true in a timely fashion. I am by no means an expert on the subject, so I might just be paranoid here. I do know though that the use of a volatile bool is OK, because the value never fluctuates it stays false until someone sets it to true. It can't go from false to true to false. No locks are used, nor are any expensive kernel objects used, so I think it's the most correct and performant way to signal a cancellation. A bonus point is that the solution is PCL compliant, so it will work for store apps as well.

So to sum it up: I'd like to remove the Abort() from AlgorithmBase, and extend all algorithm interfaces with a Compute(IAlgorithmControl control) method. The default implementation non-abortable version will be left, but will default to a call to the new method, but with a "dummy object" that never throws. Would this totally mutilate GraphX, or is it worth a shot?

from graphx.

Hi, i'm not sure i understand all of this entirely :)

1. Overall idea is fine as i can see.
2. Throwing an exception will be more fastest solution by now but honestly i'm not sure about the quality. It is better to make _abortRequested public readonly property and check it, If it is true then return all the way up from the calculation. But for now we can see how it goes with an exception throwing and clean up any junk calculations after the abort.
3. Take a look at https://msdn.microsoft.com/en-us/library/system.threading.cancellationtoken(v=vs.110).aspx it is similar to that you propose as i can see :) I'm not sure it's PCL compliant sp any PCL compliant solution will be better.
4. All abort logic in algorithms are obsolete and can be freely refactored.

from graphx.

Thanks for mentioning cancellation tokens. I had just assumed they are not available for PCL assemblies. I just saw that Task is available in for PCL, which means that cancellation token (and source) is too. Awesome! Scratch all I said. The concurrency should the IMO be executed as a task with cancellation-support. I'll give it a go tonight!

from graphx.