New base::task features

[martincapello]

This PR introduces a couple of features to base::task:

• Possibility to set a new "done" callback that is called when the task completed its execution (we can change the name to completed instead of done if it makes more sense)
• New "enqueued" status, which is true when the task has been started but it is waiting in the thread pool's queue until the actual time to execute the function comes.
• New "try_skip" method, which enables the caller to remove tasks from the queue that wasn't started yet.

This was introduced to be able to do some things in aseprite/aseprite#5037. Like being able to redraw the "Running Scripts" window as needed (when a task is started/done) and to avoid showing the "stop" button for enqueued tasks. Because we can't dequeue tasks (this should be something to add to base::task and base::thread_pool as well I think)

Something to think about: should we use an atomic enum for the base::task state instead of 3 different atomic bools? I believe that a task should be only in one of the following states at any given time:

• Ready (task created but not started yet)
• Enqueued (task enqueued in thread pool's queue but not selected for execution yet)
• Running (task selected for execution and it is being executed)
• Completed (task finished execution by either success, cancellation or error)

[dacap]

Here some points:

1. If the done callback throws an exception the whole program might crash
2. Why not just doing the the "done" work from the "execute" callback?
3. Probably we could create a list of callbacks? a kind of .then().then().then() etc. I'm not sure if it makes sense but might be useful to automatically check the "cancelled" status before continuing to the next then() callback (and might improve doing tasks in the pool, but I'm not sure about this one).
4. The state could be changed to a atomic enum

[martincapello]

Here some points:

1. If the done callback throws an exception the whole program might crash

You are rigth, try catch is needed.

2. Why not just doing the the "done" work from the "execute" callback?

If a task is canceled before entering the work queue, the "done" work would never be called because the execute callback would not be executed. It might make more sense to rename it to "finished" or "completed" instead of "done".

3. Probably we could create a list of callbacks? a kind of .then().then().then() etc. I'm not sure if it makes sense but might be useful to automatically check the "cancelled" status before continuing to the next then() callback (and might improve doing tasks in the pool, but I'm not sure about this one).

I'm not sure about this either. However I would like to add an on_exception callback, to call it when an exception happens in another callback.

4. The state could be changed to a atomic enum

Great, will do it!

[martincapello]

I've added a new function "try_skip" in d32eb3f, this function would be useful to remove work from the queue that wasn't started yet.

[dacap]

To be sincere I'm not a big fan about making the base::task class more complex, I'd prefer to keep it as simple as possible (as it's the base of all tasks). It looks like now it requires some kind of documentation to know when each callback will be called (e.g. "finished" is called even when we cancel the task, or if an exception is called, etc.). The current implementation is straightforward: just one callback.

Probably we can fix some bugs to make the impl even more simple, e.g. about:

If a task is canceled before entering the work queue, the "done" work would never be called because the execute callback would not be executed.

We might change the current in_worker_thread to always call the execution:

void task::in_worker_thread()
{
  try {
    //if (!m_token.canceled()) // removing this line to always call the execution callback
    m_execute(m_token);
  }
  ...

About the try_skip it can be useful, we could call it try_pop() (like the event queue) or try_unschedule(). "Skip" sounds like it will be added/re-executed later or is unprioritized (the task is moved to the end of the queue), not sure if you agree with this.

The thread_pool::work is nice to make it easier to pop/cancel a enqueued task. If I remember correctly there is no need to "friend class" a subclass (so I guess the "friend class thread_pool" can be removed).

About all the other features/callbacks (finished, exception, done) I think are not necessary as they could be moved to another wrapper class/abstraction, e.g. the same RunScriptTask.

[martincapello]

Probably we can fix some bugs to make the impl even more simple, e.g. about:

If a task is canceled before entering the work queue, the "done" work would never be called because the execute callback would not be executed.

We might change the current in_worker_thread to always call the execution:

void task::in_worker_thread()
{
  try {
    //if (!m_token.canceled()) // removing this line to always call the execution callback
    m_execute(m_token);
  }
  ...

I'm fine with this, I was just trying to keep the current behavior as much as possible by adding optional new behavior.

About the try_skip it can be useful, we could call it try_pop() (like the event queue) or try_unschedule(). "Skip" sounds like it will be added/re-executed later or is unprioritized (the task is moved to the end of the queue), not sure if you agree with this.

I'm happy to change it to any other name, so try_pop() is fine by me.

The thread_pool::work is nice to make it easier to pop/cancel a enqueued task. If I remember correctly there is no need to "friend class" a subclass (so I guess the "friend class thread_pool" can be removed).

If I remove the friendship, then I got this error:
[build] /Users/martin/igarastudio/git/aseprite-beta/laf/base/thread_pool.cpp:97:42: error: 'm_func' is a private member of 'base::thread_pool::work'.
I think that the "work" class can access the private members of "thread_pool" (because of nesting), but not the other way around?

[martincapello]

Okay, trying the new changes now there is a caveat.

This is our new task::in_worker_thread:

void task::in_worker_thread()
{
  m_state = state::RUNNING;
  try {
    m_execute(m_token);    
  }
  catch (const std::exception& ex) {
    LOG(FATAL, "Exception running task: %s\n", ex.what());
  }

  m_state = state::FINISHED;
}

In my RunScriptTask class, I set the task::m_execute member to something like this:

[this, func](base::task_token& token) {
  try {
    func(m_L);
  }
  catch (const std::exception& ex) {
    // TODO: do something 
  }
  m_finished(token);
}

Now base::task::m_state is not FINISHED when my m_finished callback gets called. And I was actually removing the task when finished, but now it throws the ASSERT(m_state != state::RUNNING); in base::task::~task(). I've tried removing the assert and it works fine again...but we will agree that that is a pretty dirty trick.
So, I don't see a way to have a finished callback with the task in FINISHED state...any suggestion?