[Kimchi-devel] [PATCH] [Wok 1/2] Issue #122 - Make AsyncTask stoppable.

[Paulo Ricardo Paz Vital]

On Aug 31 01:51PM, Aline Manera wrote:
> 
> 
> On 08/30/2016 10:57 AM, pvital at linux.vnet.ibm.com wrote:
> > From: Paulo Vital <pvital at linux.vnet.ibm.com>
> > 
> > This patch gives to user a way to 'stop' a Task that is still running by setting
> > the Task status to "killed".
> > 
> > Since an AsyncTask is basic a thread running in the system and this thread can
> > execute a pure Python method or a background command (by using run_command()
> > from wok.utils), the developer must pass to AsyncTask constructor a method to be
> > executed by the DELETE operation, called here as 'kill_cb'. If none kill_cb is
> > passed, the task will not be able to stopped and an error message will be raised
> > to user if DELETE operation is executed. Otherwise, the kill_cb method will be
> > executed by kill() method (responsible to execute the DELETE operation) of
> > AsyncTask class and its status set to 'killed'.
> > 
> > Signed-off-by: Paulo Vital <pvital at linux.vnet.ibm.com>
> > ---
> >   docs/API/tasks.md      |  2 ++
> >   src/wok/asynctask.py   | 18 +++++++++++++++++-
> >   src/wok/i18n.py        |  1 +
> >   src/wok/model/tasks.py | 13 +++++++++++++
> >   4 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/API/tasks.md b/docs/API/tasks.md
> > index e0c404f..2068f29 100644
> > --- a/docs/API/tasks.md
> > +++ b/docs/API/tasks.md
> > @@ -27,8 +27,10 @@ server.
> >           * running: The task is running
> >           * finished: The task has finished successfully
> >           * failed: The task failed
> > +        * killed: The task was killed by user
> >       * message: Human-readable details about the Task status
> >       * target_uri: Resource URI related to the Task
> > +* **DELETE**: Kill the Task, moving its status to 'killed'
> >   * **POST**: *See Task Actions*
> > 
> >   **Actions (POST):**
> > diff --git a/src/wok/asynctask.py b/src/wok/asynctask.py
> > index b98ad9a..4a4ee32 100644
> > --- a/src/wok/asynctask.py
> > +++ b/src/wok/asynctask.py
> > @@ -26,6 +26,9 @@ import traceback
> >   import uuid
> > 
> > 
> > +from wok.exception import OperationFailed
> > +
> > +
> >   tasks_queue = {}
> > 
> > 
> > @@ -41,10 +44,11 @@ def clean_old_tasks():
> > 
> > 
> >   class AsyncTask(object):
> > -    def __init__(self, target_uri, fn, opaque=None):
> > +    def __init__(self, target_uri, fn, opaque=None, kill_cb=None):
> >           self.id = str(uuid.uuid1())
> >           self.target_uri = target_uri
> >           self.fn = fn
> > +        self.kill_cb = kill_cb
> >           self.status = 'running'
> >           self.message = 'The request is being processing.'
> >           self.timestamp = time.time()
> > @@ -79,3 +83,15 @@ class AsyncTask(object):
> >           except KeyError:
> >               msg = "There's no task_id %s in tasks_queue. Nothing changed."
> >               cherrypy.log.error_log.error(msg % self.id)
> > +
> > +    def kill(self):
> > +        if self.kill_cb is None:
> > +            msg = 'There is no callback to execute DELETE operation.'
> > +            raise OperationFailed('WOKASYNC0002E', {'err': msg})
> > +
> 
> It should be "InvalidOperation" exception.

OK!
/me never knows which exception is the best!!

> 
> > +        try:
> > +            self.kill_cb()
> > +            self.status = 'killed'
> > +            self.message = 'Task killed by user.'
> > +        except Exception, e:
> > +            raise OperationFailed('WOKASYNC0002E', {'err': e.message})
> 
> I see you are using WOKASYNC0002E with a placeholder for 'err' but in both
> cases the error message is manually set in the code.
> 
> msg = 'There is no callback to execute DELETE operation.'
> self.message = 'Task killed by user.'
> 
> I'd suggest to create 2 different error codes, one for each case above, so
> the entire message can be translated.
> Otherwise, the error message will keep in English and we can translate it to
> the language the user sets.
> 

Here I'm raising the exception message. The self.message is the AsyncTask 
message field and will not be used here. 

I got your point, so I'll create one specific message to be used to 
msg = 'There is no callback to execute DELETE operation.' and let
WOKASYNC0002E as is, because the idea is inform the exception error.

> 
> > diff --git a/src/wok/i18n.py b/src/wok/i18n.py
> > index ade2ae9..937477f 100644
> > --- a/src/wok/i18n.py
> > +++ b/src/wok/i18n.py
> > @@ -34,6 +34,7 @@ messages = {
> >       "WOKAPI0009E": _("You don't have permission to perform this operation."),
> > 
> >       "WOKASYNC0001E": _("Unable to find task id: %(id)s"),
> > +    "WOKASYNC0002E": _("Unable to kill task due error: %(err)s"),
> >       "WOKASYNC0003E": _("Timeout of %(seconds)s seconds expired while running task '%(task)s."),
> > 
> >       "WOKAUTH0001E": _("Authentication failed for user '%(username)s'. [Error code: %(code)s]"),
> > diff --git a/src/wok/model/tasks.py b/src/wok/model/tasks.py
> > index 6a2b4bf..8ae1ce4 100644
> > --- a/src/wok/model/tasks.py
> > +++ b/src/wok/model/tasks.py
> > @@ -68,3 +68,16 @@ class TaskModel(object):
> > 
> >           raise TimeoutExpired('WOKASYNC0003E', {'seconds': timeout,
> >                                                  'task': task.target_uri})
> > +
> > +    def delete(self, id):
> > +        """
> > +        'Stops' an AsyncTask, by executing the kill callback provided by user
> > +        when created the task. Task's status will be changed to 'killed'.
> > +        """
> > +        try:
> > +            task = tasks_queue[id]
> > +        except KeyError:
> > +            raise NotFoundError("WOKASYNC0001E", {'id': id})
> > +
> > +        if task.status is 'running':
> > +            task.kill()
> 

-- 
Paulo Ricardo Paz Vital
Linux Technology Center, IBM Systems
http://www.ibm.com/linux/ltc/