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

[Aline Manera]

On 08/31/2016 02:22 PM, Paulo Ricardo Paz Vital wrote:
> 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.

Ok.

About the message:

self.message = 'Task killed by user.'


Maybe it is better to fill it will the exception raised too.
I am afraid of the UI uses this message someway and it will not be 
translated.

self.message = e.message

>>> 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()