Safer way of cancelling threads

[ghost]

I'm not sure cancel should be in the Thread's interface now. I think we should implement cancelling logic in the client code and use a flag to just exit the thread function, without relying on the OS facilities (which are not always there or are unsafe). Writing non-blocking client code should be preferred over forcing thread termination.

What I mean is, there is no safe way to terminate code such as

thread := Thread new(func { while(1) doSomething() })

The client code should provide some cancellation points. Ideally every thread task could be a subclass of some abstract Task interface like

AbstractThreadTask: abstract class {
    run: abstract func
    cancel: virtual func { lock(), this _runFlag = false, unlock() }
    isCancelled: virtual func -> Bool { lock(), result := this _runFlag, unlock(), return result }
}

and clients implementing the interface should check if the task was cancelled inside the run implementation, so the previous "unsafe" example becomes

MyThreadTask: class extends AbstractThreadTask {
    run: override func {
        while(1){ 
           if( this isCancelled() ) {
               doCleanup()
               return
          }
          doSomething()
      }
    }
}
thread := Thread new(MyThreadTask new())

and now the Thread class can have a safer cancel implementation, which does not rely on the OS:

Thread: abstract class {
    _task: AbstractThreadTask
    _code: func { this _task run() }
    cancel: func {
       this _task cancel() // simply sets the flag, should be safe to join or detach the thread now
    }
   ...
}

[ btw. Ideally we'd like to define the task subclasses in place, like in Java :) ]

I'm aware that this idea makes the Thread class less low-level and maybe slightly less convenient to use (but simple tasks could be automatically wrapped around the ThreadTask object, the use of cancellation points should be encouraged, not enforced).

[marcusnaslund]

What about an API like this?

threadFunc := func {
    while (true) {
        if (Thread currentThread() isCanceled()) // or just Thread isCanceled()
            break
        doSomeHeavyTask()
    }
}

thread := Thread new(threadFunc) . start()
Time sleepSec(10)
thread cancel()

The cancel() method sets a flag in Thread instead of calling OS/pthread stuff.

[ghost]

So this requires every thread instance to have a mutex ? I was thinking to keep it separated in the abstract task layer and even allow running short lived tasks directly via OS api, without the possibility to cancel them (and without allocating mutexes for them). I like the simplicity of the api though, maybe we could start with it.

[marcusnaslund]

So this requires every thread instance to have a mutex ?

Does it, though? The flag will only change to true regardless of what it was before. We wouldn't get perfect sync when reading isCanceled(), but then again we don't really get that now either (or require it), do we?

The advantage is that it's really simple to implement, and really easy to use for example in the ThreadPool (cancel wouldn't force the ThreadPool thread to exit, just to stop the current task, then its cancel flag would be set to false before moving on to the next task in the queue).

[ghost]

We wouldn't get perfect sync when reading isCanceled(), but then again we don't really get that now either (or require it), do we?

Using shared data without a lock just seems lazy, just like driving off the motorway without using the blinker. It works, but is it really how things should be done ? Let's start with your api but also a mutex for the cancel flag in some experimental branch. Then we can evaluate how costly it is and maybe remove it, what do you say ?

[marcusnaslund]

like driving off the motorway without using the blinker

+1

[marcusnaslund]

Alright, we have a decision to implement my approach. The Thread constructor will take one of three arguments, either no mutex is available (meaning cancel will not be available), or a Mutex instance is supplied, or a Mutex is created and owned by the Thread instance.

A Thread instance will have cancel() and isCanceled() methods, and a canceled boolean flag + cancelMutex + ownsCancelMutex members.

[marcusnaslund]

Alright, after examination, it seems my approach won't work properly without either: 1) A global thread table, which seems ugly. 2) Having cancelable threads take a Func (Thread) instead of just a Func, where the argument is the running thread, as per @emilwestergren 's suggestion

A third option is @sebastianbaginski 's approach: 3) A ThreadTask and a CancelableThreadTask (which inherits from the former). The task system in ThreadPool can utilize this as well. I think creating a thread using a func should also still work, and it would take care of creating (and disposing) a ThreadTask that runs the function. Likewise with the ThreadPool and _ThreadPromise/_ThreadFuture.

@fredrikbryntesson @thomasfanell ?

[ghost]

Another question: should cancel be in the Thread interface ? Looks like there is no reliable, platform-independent way of cancelling the threads, so maybe we should drop this concept and talk about cancelling running tasks instead ?

[marcusnaslund]

Agreed. That would be consistent with the ThreadPool and Promise/Future.