remove task's c interface #48

shengofsun · 2018-05-08T09:53:34Z

No description provided.

Conflicts: include/dsn/cpp/clientlet.h include/dsn/cpp/task_helper.h include/dsn/tool/cli/cli.client.h src/apps/nfs/nfs_client_impl.cpp src/apps/nfs/nfs_server_impl.cpp src/core/core/rpc_engine.cpp src/core/core/service_api_c.cpp src/dist/replication/lib/mutation_log.cpp src/dist/replication/lib/mutation_log.h

… & rename reset_callback to replace_callback for rpc_response_task Conflicts: src/core/tests/clientlet.cpp

…succeed and reset callback when timer task is cancelled

shengofsun · 2018-05-08T09:54:03Z

你们这周慢慢看，我先上集群测试。

neverchanje · 2018-05-09T01:52:49Z

建议参考 https://github.com/cloudera/kudu/blob/master/src/kudu/gutil/ref_counted.h 的 scoped_refptr 来修改 ref_ptr，并且有必要引入 https://github.com/chromium/chromium/blob/07eea964c3f60f501782d8eb51f62ca75ddf3908/base/memory/ref_counted_unittest.cc 来保证修改的正确性，不能只依靠 killtest。

neverchanje · 2018-05-09T07:14:16Z

另外其实有些对 perf counter 的改动不是很有必要，后面我会直接删掉换成新的 metrics lib。

neverchanje · 2018-05-09T07:21:48Z

看 include/dsn/tool-api/task.h 的注释。

/// Task is the asynchronous programming model in rDSN.

把 Task 叫做一个 model 显然是中式英语。它是一个 concept，是 rdsn framework 的一个 component to support asynchronous programming model，是一个 execution piece。另外介绍的时候还引入了 future 和 promise 这两个多数人不是很熟悉的无关概念，把事情变得更糟了。

/// In order to support these features, a state called "task_state" is kept in each task,
/// the transition among different states are:

这句话有语病，英文给人的意思是 “我们通过在每个 task 里存 task_state 就能支持这些 feature”，显然是不对的。并且我没有看出介绍 state transition 的必要性，用户（至少我用到现在）只需要知道 task 可以 enqueue，可以 cancel，可以 wait 即可。

c# 有 https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/task-based-asynchronous-programming ，也是用的 Task 抽象。你可以借鉴一下 TPL 的文档，看看改进一下文档？因为 Task 对 rDSN 很关键，可以多打磨一下注释。

关于 hook points 的介绍还是不错的。

qinzuoyan · 2018-05-10T03:44:26Z

include/dsn/cpp/clientlet.h

                         task_tracker *tracker,
-                         TCallback &&callback,
+                         TCallback &&cb,


为什么这里将 callback 改为 cb，但是上面和下面函数都不改？

改回来了

qinzuoyan · 2018-05-10T06:38:18Z

include/dsn/cpp/clientlet.h

+call(dsn::rpc_address server,
+     dsn::task_code code,
+     TRequest &&req,
+     dsn::task_tracker *owner,


owner -> tracker

qinzuoyan · 2018-05-10T07:06:52Z

src/core/core/service_engine.h

-                              const char *extra_name,
-                              const dsn_rpc_request_handler_t &h);
+    bool
+    rpc_register_handler(dsn::task_code code, const char *extra_name, const rpc_request_handler &h);


dsn::前缀可去掉，下一个方法也是

qinzuoyan · 2018-05-10T07:35:04Z

include/dsn/c/api_layer1.h

-extern DSN_API void
-dsn_file_write(dsn_handle_t file, const char *buffer, int count, uint64_t offset, dsn_task_t cb);
+extern DSN_API void dsn_file_write(
+    dsn_handle_t file, const char *buffer, int count, uint64_t offset, dsn::aio_task *cb);


不传裸指针： const dsn::aio_task_ptr &cb
其他同

后面留洋有个专门的pr改这个

qinzuoyan · 2018-05-10T07:39:01Z

include/dsn/c/api_task.h

-typedef void (*dsn_task_cancelled_handler_t)(
-    void * ///< shared with the task handler callbacks, e.g., in \ref dsn_task_handler_t
-    );
+typedef std::function<void()> task_handler;


这几个handler我觉得放在dsn namespace下更合适。而且它们都没有dsn_前缀。
目前来说，只有dsn_前缀的（通常是C接口）不需要放在dsn namespace下面。

既然是移除C接口，我觉得c/api_task.h 这个头文件都应当不需要了

移到dsn的namespace之下了。

另外，就现在的目录结构，把这个文件全移了，include改起来会麻烦些。后面会有个专门的pr把这个文件移除掉。

qinzuoyan · 2018-05-10T08:33:15Z

src/core/core/network.cpp

+                start_read_next();
+                this->release_ref();
+            }));
+            delay_task->enqueue();


感觉这里是个bug：delay_ms 没有被使用？

qinzuoyan · 2018-05-10T08:41:31Z

src/core/core/rpc_engine.h

-        rpc_response_task *resp_task;
-        task *timeout_task;
+        rpc_response_task_ptr resp_task;
+        task_ptr timeout_task;


这里直接用 rpc_timeout_task_ptr 更好

rpc_timeout_task是在cpp里面定义的，如果换了得把它挪到头文件里来。而且timeout_task没有定义新的函数，没有必要把子类的类定义暴露出来

qinzuoyan · 2018-05-10T08:42:06Z

src/core/core/rpc_engine.cpp

@@ -262,7 +256,10 @@ void rpc_client_matcher::on_rpc_timeout(uint64_t key)
    resend = (now_ts_ms < timeout_ts_ms && call->state() == TASK_STATE_READY);

    // TODO: memory pool for this task
-    task *new_timeout_task = resend ? new rpc_timeout_task(this, key, call->node()) : nullptr;
+    task_ptr new_timeout_task;


这里直接就是 rpc_timeout_task_ptr

和上面一样，因为rpc_timeout_task没有新的函数，所以没必要写子类指针

qinzuoyan · 2018-05-10T08:45:11Z

src/core/core/rpc_engine.cpp

+                                static_cast<int>(deadline_ms - nms - gap);
+                            // current task must be rpc_response_task, and now we are in it's body
+                            rpc_response_task_ptr ctask =
+                                dynamic_cast<rpc_response_task *>(task::get_current_task());


加上assert语句：
dassert(ctask != null, "current task must be rpc_response_task");
这样上面的注释也不需要了。

qinzuoyan · 2018-05-10T08:48:26Z

src/core/core/rpc_engine.h

-                              const char *extra_name,
-                              const dsn_rpc_request_handler_t &h);
+    bool
+    register_rpc_handler(dsn::task_code code, const char *extra_name, const rpc_request_handler &h);


dsn::前缀没必要

qinzuoyan · 2018-05-10T09:24:02Z

src/dist/replication/zookeeper/meta_state_service_zookeeper.cpp

@@ -203,7 +203,7 @@ task_ptr meta_state_service_zookeeper::create_node(const std::string &node,
                                                   const blob &value,
                                                   dsn::task_tracker *tracker)
 {
-    task_ptr tsk = tasking::create_late_task(cb_code, cb_create, 0, tracker);
+    task_ptr tsk = tasking::create_late_task<err_callback>(cb_code, cb_create, 0, tracker);


为什么要加<err_callback>，编译器不能自动推导吗？

qinzuoyan · 2018-05-10T09:25:56Z

src/dist/replication/test/simple_kv/client.cpp

@@ -127,15 +127,15 @@ void simple_kv_client_app::begin_write(int id,
           key.c_str(),
           value.c_str(),
           timeout_ms);
-    std::unique_ptr<write_context> ctx(new write_context());
+    std::shared_ptr<write_context> ctx(new write_context());


unique_ptr 改为 shared_ptr 的原因和意义是什么？莫非是为了让你的IDE容易识别？

和我修改了rpc_call那陀复杂的模板有关系。其实也没看看明白，为啥unique_ptr编译不过……
简单来说，callback会调用一次赋值操作，但是unique_ptr的赋值操作被禁用了……

qinzuoyan · 2018-05-10T09:39:34Z

src/dist/replication/test/meta_test/unit_test/backup_test.cpp

@@ -133,7 +133,8 @@ class progress_liar : public meta_service
 {
 public:
    // req is held by callback, we don't need to handle the life-time of it
-    virtual void send_request(dsn_message_t req, const rpc_address &target, task_ptr callback)
+    virtual void
+    send_request(dsn_message_t req, const rpc_address &target, rpc_response_task_ptr callback)


传 const rpc_response_task_ptr &callback 比较好，基类都改了吧

qinzuoyan · 2018-05-10T09:43:18Z

src/dist/replication/meta_server/meta_service.h

@@ -100,9 +100,10 @@ class meta_service : public serverlet<meta_service>
    {
        dsn_rpc_call_one_way(target, request);
    }
-    virtual void send_request(dsn_message_t /*req*/, const rpc_address &target, task_ptr callback)
+    virtual void
+    send_request(dsn_message_t /*req*/, const rpc_address &target, rpc_response_task_ptr callback)


rpc_response_task_ptr callback 改为 const rpc_response_task_ptr &callback

qinzuoyan · 2018-05-10T09:53:32Z

src/dist/replication/lib/replica_learn.cpp

@@ -1390,14 +1391,14 @@ error_code replica::apply_learned_state_from_private_log(learn_state &state)
    // temp prepare list for learning purpose
    prepare_list plist(_app->last_committed_decree(),
                       _options->max_mutation_count_in_prepare_list,
-                       [this, &err](mutation_ptr &mu) {
+                       [this](mutation_ptr &mu) {


这里传err进去本想是检查 _app->apply_mutation(mu); 的返回值的。
不知道为什么没有检查，可能是个漏洞。
加个TODO在 _app->apply_mutation(mu); 那里吧。

qinzuoyan · 2018-05-10T09:54:37Z

src/dist/replication/lib/replica_context.cpp

@@ -52,7 +52,7 @@ namespace replication {
        if (t != nullptr) {                                                                        \
            bool finished;                                                                         \
            t->cancel(force, &finished);                                                           \
-            if (!finished && !dsn_task_is_running_inside(task_->native_handle()))                  \
+            if (!finished && !dsn_task_is_running_inside(task_.get()))                             \


dsn_task_is_running_inside() 传ptr参数更好？

如果改成ptr, 需要include <dsn/tool-api/task.h>，现在头文件的结构还不是很清晰，直接include这个所涉及的事情会多一些。我后面会有pr来整理这些内容

qinzuoyan · 2018-05-10T10:01:02Z

src/dist/replication/lib/replica_config.cpp

@@ -459,7 +459,8 @@ void replica::on_update_configuration_on_meta_server_reply(
                                err, request, response, std::move(req2));
                        },
                        get_gpid().thread_hash());
-                    dsn_rpc_call(target, _primary_states.reconfiguration_task->native_handle());
+                    dsn_rpc_call(target, t.get());
+                    _primary_states.reconfiguration_task = t;


通常来说， _primary_states.reconfiguration_task = t; 放在 dsn_rpc_call(target, t.get()); 前面会比较安全。

qinzuoyan · 2018-05-10T10:39:30Z

src/dist/replication/ddl_lib/replication_ddl_client.cpp

@@ -1307,18 +1307,22 @@ bool replication_ddl_client::valid_app_char(int c)
    return (bool)std::isalnum(c) || c == '_' || c == '.' || c == ':';
 }

-void replication_ddl_client::end_meta_request(
-    task_ptr callback, int retry_times, error_code err, dsn_message_t request, dsn_message_t resp)
+void replication_ddl_client::end_meta_request(rpc_response_task_ptr &&callback,


感觉这里用右值引用的必要性不是很大，可以考虑直接用const rpc_response_task_ptr &callback，提高代码可读性

qinzuoyan · 2018-05-11T01:26:11Z

src/dist/replication/lib/replica_config.cpp

@@ -659,14 +660,13 @@ bool replica::update_local_configuration(const replica_configuration &config,
        if (config.status != partition_status::PS_SECONDARY &&
            config.status != partition_status::PS_ERROR) {
            if (!_secondary_states.cleanup(false)) {
-                dsn_task_t native_handle;
+                dsn::task *native_handle;


这一坨的代码好像没多大必要，而且日志打印有问题，加个TODO吧

加了TODO, 日志貌似没什么问题？

qinzuoyan · 2018-05-11T01:30:42Z

src/core/tests/service_api_c.cpp

-                                                  &rin,
-                                                  0);
-        dsn_task_add_ref(tin);
+        dsn::aio_task *tin = new dsn::aio_task(LPC_AIO_TEST_READ,


可以考虑用 aio_task_ptr，下同

这里测试就是用裸的指针，和原来的测试保持原汁原味的。
anyway, 我移了吧。

qinzuoyan · 2018-05-11T01:52:34Z

include/dsn/c/api_task.h

-typedef void (*dsn_task_cancelled_handler_t)(
-    void * ///< shared with the task handler callbacks, e.g., in \ref dsn_task_handler_t
-    );
+typedef std::function<void()> task_handler;


既然是移除C接口，我觉得c/api_task.h 这个头文件都应当不需要了

qinzuoyan · 2018-05-11T01:59:33Z

include/dsn/dist/replication/replication_ddl_client.h

+            msg,
+            &_tracker,
+            [this, task](error_code err, dsn_message_t request, dsn_message_t response) mutable {
+                end_meta_request(std::move(task), 0, err, request, response);


这里用std::move()是有风险的，如果callback是在下面的“return task”之前执行，那么return的就是nullptr。
这也是我为什么不建议 end_meta_request() 传右值引用的原因。
传右值引用，好处只是减少一些情况下对ptr的copy，减少一些atomic操作。（实际上传 const ptr &已经避免了大部分的copy）。但是右值引用会让使用者不得不小心数据的生命周期，还可能引入bug。权衡起来，并不划算。

改了，不过这个地方貌似没有风险？因为task是值传递进去的。

话说，旧代码用的就是move，也不知道是你写的还是晓彤写的……

那么只能说旧代码也是有问题的。。。

qinzuoyan · 2018-05-11T02:03:14Z

include/dsn/utility/autoref_ptr.h

@@ -100,15 +100,36 @@ class ref_ptr
            _obj->add_ref();
    }

+    template <typename U>


既然有了这个模板，实际上 ref_ptr(T *obj) 函数就不需要了。这样可以避免重复代码。
可以参考std::shared_ptr的实现： __shared_ptr::__shared_ptr(_Tp1* __p)

下同

改了会编译不过，有了上面的那个函数，nullptr会隐式的实例化成上面的函数调用。shared_ptr能这么写，是因为shared_ptr显示的实现了std::shared_ptr(std::nullptr_t )这个函数。

qinzuoyan · 2018-05-11T02:11:48Z

include/dsn/dist/failure_detector/fd.client.h

@@ -36,6 +36,8 @@
 #pragma once
 #include "fd.code.definition.h"
 #include <iostream>
+#include <dsn/utility/optional.h>


我看用了 empty_rpc_handler 的地方，头文件都要加 <dsn/utility/optional.h>，感觉很繁琐，难道不能在定义 empty_rpc_handler 的头文件中加一次 <dsn/utility/optional.h> 吗？

optional提供的是unwrap_or, empty_rpc_handler提供的是empty_rpc_handler，这两个是没有关系的。只不过是我们在rpc_call这里，两个恰好都用了。

qinzuoyan · 2018-05-11T02:18:21Z

include/dsn/tool-api/task.h

+    aio_task(dsn::task_code code, aio_handler &&cb, int hash = 0, service_node *node = nullptr);
+    ~aio_task();
+
+    void enqueue_aio(error_code err, size_t transferred_size);


为什么要改名字为 enqueue_aio() ，我感觉直接使用 enqueue() 更好，这里函数参数不同，可以重载。
况且 rpc_response_task 也是用的 enqueue() ，最好保持一致。

我暂时先把这个改回来了，后面统一把这些接口改成enqueue_with吧

qinzuoyan · 2018-05-11T02:20:23Z

src/apps/nfs/nfs_server_impl.cpp

-        this,
-        cp_cap = std::move(cp)
-    ](error_code err, int sz) mutable { internal_read_callback(err, sz, cp_cap); });
+    std::shared_ptr<callback_para> cp = std::make_shared<callback_para>(std::move(reply));


这里改为用shared_ptr来new对象的考虑是什么？

因为改了函数接口，现在为了清晰，函数只接受一个aio_handler。而以前是一个模板，可以输入任意类型的handler, 只要这个handler的输入参数是(err, size)就可以了。

而这个callback, 不是个aio_handler，用以前的方式会实例化一个新的模板函数出来。现在会以值拷贝的方式转换成aio_handler。但这种方式会报错，因为callback_para是不能拷贝的。

qinzuoyan · 2018-05-11T02:27:45Z

include/dsn/cpp/clientlet.h

-                 TCallback &&callback,
-                 int hash = 0,
-                 std::chrono::milliseconds delay = std::chrono::milliseconds(0))
+inline task_ptr enqueue_timer(dsn::task_code evt,


既然是 enqueue_timer() ，那返回值为 timer_task_ptr 更好

和上面一样，timer task没有新的函数引入，所以就返回了基类指针

qinzuoyan · 2018-05-11T02:29:44Z

include/dsn/cpp/clientlet.h

-task_ptr
-create_aio_task(dsn::task_code callback_code, task_tracker *tracker, TCallback &&callback, int hash)
+inline aio_task_ptr
+create_aio_task(dsn::task_code code, task_tracker *tracker, aio_handler &&callback, int hash)


dsn::前缀可以去掉。
提供默认值要和其他函数保持一致：int hash = 0；

改了，不过之前这个地方貌似就没有默认值

shengofsun · 2018-05-11T06:29:47Z

@neverchanje 关于ref counter, kudu的看上去好像挺大一陀，而且改起来要改好多代码，下次单独提一个pr看怎么改吧。比如说看看能不能直接用boost_intrusive_ptr

shengofsun · 2018-05-11T06:30:42Z

@neverchanje metrics lib合进来之后，该删的直接删好了

qinzuoyan · 2018-05-11T03:52:39Z

include/dsn/tool-api/task.h

+/// "finished" state after running of callbacks.
+///
+/// But for timer tasks, the state will transit to "ready" again after "running"
+/// as the callback are executed periodically.


as the task need to be executed periodically.

qinzuoyan · 2018-05-11T03:54:54Z

include/dsn/tool-api/task.h

+/// But for timer tasks, the state will transit to "ready" again after "running"
+/// as the callback are executed periodically.
+///
+/// "Data consumer" can cancel the executation of "ready" tasks with "cancel". However, if a task


with method cancel().

qinzuoyan · 2018-05-11T03:56:11Z

include/dsn/tool-api/task.h

+/// as the callback are executed periodically.
+///
+/// "Data consumer" can cancel the executation of "ready" tasks with "cancel". However, if a task
+/// is in not in "ready" state, the cancel will fail.


the cancel will fail (returning false).

qinzuoyan · 2018-05-11T03:57:55Z

include/dsn/tool-api/task.h

+/// is in not in "ready" state, the cancel will fail.
+///
+/// Another key design in rDSN is that we add some "hook points" when the state is in transition,
+/// like "on_task_create", "on_task_enqueue", "on_task_dequeue". We can execute different functions


like "on_task_create", "on_task_enqueue", "on_task_begin", "on_task_end", "on_task_canceled", and so on.

没写特别多，反正是like 和 and so on

qinzuoyan · 2018-05-11T04:02:51Z

include/dsn/tool-api/task.h

+    //
+    // if finished isn't nullptr, it will be used to indicate
+    // whether the task is finished/cancelled, that is to say:
+    //    *finished == true <- the task has been finished/cancelled after the function return


finished == true: the task has been finished/cancelled when this method returns

qinzuoyan · 2018-05-11T07:08:20Z

include/dsn/tool-api/task.h

+    // return:
+    //    true : change task state from TASK_STATE_RUNNING to TASK_STATE_READY succeed
+    //    false : change task state failed
+    bool set_retry(bool enqueue_immediately = true);


我看enqueue_immediately这个参数没有必要了，直接干掉吧

现在rpc_response_task和timer_task传的值不一样。

后面把rpc_response_task重试的逻辑移除了之后，我再来干这个参数吧。

qinzuoyan · 2018-05-11T07:25:01Z

include/dsn/tool-api/task.h

+/// like "on_task_create", "on_task_enqueue", "on_task_dequeue". We can execute different functions
+/// for different purposes on these hook points, you may want to refer to
+/// "tracer", "profiler" and "fault_injector" for details.
+///


增加注释，说明不能依赖callback执行资源释放的事情，不然可能会因为callback得不到执行（譬如cancel）而导致资源泄露

qinzuoyan · 2018-05-11T07:26:08Z

include/dsn/tool-api/task.h

+    // wait until a task to finished/cancelled
+    bool wait(int timeout_milliseconds = TIME_MS_MAX);
+
+    void exec_internal();


增加注释，表明这是内部使用的函数，而不是给用户使用的函数。

qinzuoyan · 2018-05-11T07:37:32Z

include/dsn/tool-api/task.h

+        : task(code, hash, node), _cb(std::move(cb))
+    {
+    }
+    virtual ~raw_task() override { clear_callback(); }


没必要调用 clear_callback();
下同。

qinzuoyan · 2018-05-11T07:39:54Z

include/dsn/tool-api/task.h

               uint32_t interval_milliseconds,
               int hash = 0,
               service_node *node = nullptr);
+    timer_task(dsn::task_code code,
+               task_handler &&cb,
+               uint32_t interval_milliseconds,


这类时间统一都用 int 类型吧。
譬如 void set_delay(int delay_milliseconds = 0); 也是int类型。

qinzuoyan

LGTM

LoveHeat · 2018-05-15T09:39:21Z

include/dsn/tool-api/task.h

+/// So please take care when you call cancel. Memory leak may occur if you don't pay attention.
+/// For example:
+///
+/// int a = new int(5);


int * a = new int(5);

neverchanje · 2018-05-16T06:52:35Z

task 的注释简单改了一些东西，后面计划把这些注释放到 docs 下，把它们放在头文件非常主次不明。代码注释不应该直接当作文档。有些内容一大段写起来虽然辛苦，但是还是不够规范。

改了一些语法问题（貌似你的 IDE 不支持拼写提示），去掉 future promise 这个多余的解释（最起码也要用 coroutine，好歹是同一个东西）。

///
/// Task is a thread-like execution piece that is much lighter than a normal thread.
/// Huge number of tasks may be hosted by a small number of actual threads run within
/// a thread pool.
///
/// When creating the task, user must use 3 parameters to specify in which thread the
/// callback should run:
///
///     1. node: specifies the computation engine of the callback, i.e, the "pool" of "thread_pool"
///     2. task_code: a index to the "task_spec". task_spec specifies which thread pool of
///        the computation engine to run the callback. some other task information is also
///        recorded in task_spec. please refer to @task_code, @task_spec, @thread_pool_code
///        for more details.
///     3. hash: specifies which thread in the thread pool to execute the callback. (This is
///        somewhat not accurate, coz "hash" will work together with thread_pool's "partition"
///        option. Please refer to @task_worker_pool for more details).
///
/// So the running thread of callback will be determined hierarchically:
///
///         |<---determined by "node"
///         |        |<-----determined by "code"
///         |        |       |-------------determined by "hash"
///         |        |       |
///  |------V--------|-------|---------------------------|     |--------------|
///  | |-------------V-------|-----|     |-------------| |     |              |
///  | | |--------|     |----V---| |     |             | |     |              |
///  | | | thread | ... | thread | |     |             | |     |              |
///  | | |--------|     |--------| |     |             | |     |              |
///  | |       thread pool         | ... | thread pool | |     |              |
///  | |---------------------------|     |-------------| |     |              |
///  |                 service node                      | ... | service node |
///  |---------------------------------------------------|     |--------------|
///
/// A status value called "task_state" is kept in each task to indicate the task running state,
/// the transition among different states are:
///
///                      |
///              (new created task)
///                      |
///                      V
///      |-----------> ready------------|
///      |               |              |
/// (timer task)     (execute)       (cancel)
///      |               |              |
///      |               V              V
///      |------------ running      cancelled
///                      |
///                (one shot task)
///                      |
///                      V
///                   finished
///
/// As shown above, a new created task will be in "ready" state. After obtaining the data
/// (like a rpc gets the response, or a disk io succeeds), the data provider module will store
/// proper value into the task and dispatch it to some thread to execute the task with "enqueue".
///
/// After a task is "dequeued" from thread, it will be in "running" state.
///
/// For one shot tasks(in which callback can ONLY be executed ONCE),  the task will be in
/// "finished" state after running of callbacks.
///
/// But for timer tasks, the state will transit to "ready" again after "running"
/// as the callback need to be executed periodically.
///
/// The callers can cancel the execution of "ready" tasks with method "cancel". However,
/// if a task is in not in "ready" state, the cancel will fail (returning false).
///
/// So from the perspective of task user, a created task can only be controlled by "enqueue" or
/// "cancel". An "enqueue" operation will make the callback to execute at some time in the future,
/// and an "cancel" operation will prevent the callback from executing.
///
/// So please take care when you call cancel. Memory leak may occur if you don't pay attention.
/// For example:
///
/// int *a = new int(5);
/// raw_task t = new raw_task(code, [a](){ std::cout << *a << std::endl; delete a; }, hash, node);
/// t->enqueue(10_seconds_latey);
/// if (t->cancel()) {
///    std::cout << "cancel succeed, the callback will not execute and a won't be deleted"
///               << std::endl;
/// }
///
/// In order to prevent this, we recommend you to pass RAII objects to callback:
///
/// std::shared_ptr<int> a = std::make_shared<int>(5);
/// raw_task t = new raw_task(code, [a]() { std::cout << *a << std::endl; }, hash, node);
///
/// In this case, the callback object will be destructed if t is cancelled.
///
/// Another key design in rDSN is that we add some "hook points" when the state is in transition,
/// like "on_task_create", "on_task_enqueue", "on_task_dequeue", etc. We can execute different
/// functions for different purposes on these hook points, you may want to refer to
/// "tracer", "profiler" and "fault_injector" for details.
///

neverchanje · 2018-05-16T07:11:24Z

几个 handler 应该要加注释，它们都是比较重要的接口

typedef std::function<void()> task_handler;

/// A callback to handle rpc requests.
///
/// Parameters:
///  - dsn_message_t: the received rpc request
typedef std::function<void(dsn_message_t)> rpc_request_handler;

/// A callback to handle rpc responses.
///
/// Parameters:
///  - error_code
///  - dsn_message_t: the sent rpc request
///  - dsn_message_t: the received rpc response
typedef std::function<void(error_code, dsn_message_t, dsn_message_t)> rpc_response_handler;

/// Parameters:
///  - error_code
///  - size_t: the read or written size of bytes from file.
typedef std::function<void(error_code, size_t)> aio_handler;

qinzuoyan

LGTM

neverchanje

LGTM

shengofsun and others added 3 commits May 7, 2018 17:00

task: reset cb/handler to avoid memory leak after task exec or cancel…

bb93e72

… & rename reset_callback to replace_callback for rpc_response_task Conflicts: src/core/tests/clientlet.cpp

bug fix: task::cancel() call reset_callback base whether cancel task …

0f6e3c7

…succeed and reset callback when timer task is cancelled

shengofsun requested review from qinzuoyan, neverchanje, acelyc111 and LoveHeat May 8, 2018 09:54

neverchanje closed this May 9, 2018

neverchanje reopened this May 9, 2018

qinzuoyan reviewed May 10, 2018

View reviewed changes

qinzuoyan reviewed May 11, 2018

View reviewed changes

shengofsun added 3 commits May 14, 2018 18:32

task: fix based on comments

0273bd1

task: fix based on comments

7aaf8d5

task: remove dsn namespace for task_code

8bb543f

qinzuoyan previously approved these changes May 15, 2018

View reviewed changes

LoveHeat reviewed May 15, 2018

View reviewed changes

task: fix based on code review

3d1be9c

shengofsun dismissed qinzuoyan’s stale review via 3d1be9c May 16, 2018 08:28

qinzuoyan approved these changes May 16, 2018

View reviewed changes

Merge branch 'master' into master

6d66f71

neverchanje approved these changes May 16, 2018

View reviewed changes

shengofsun merged commit 3dbda51 into XiaoMi:master May 16, 2018

qinzuoyan mentioned this pull request Aug 20, 2018

代码重构与化简 #141

Open

28 tasks

neverchanje added the type/refactoring label Sep 12, 2018

remove task's c interface #48

remove task's c interface #48

Conversation

shengofsun commented May 8, 2018

shengofsun commented May 8, 2018

neverchanje commented May 9, 2018

neverchanje commented May 9, 2018

neverchanje commented May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shengofsun May 14, 2018 • edited Loading

Choose a reason for hiding this comment

qinzuoyan May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shengofsun May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinzuoyan May 10, 2018 • edited Loading

Choose a reason for hiding this comment

shengofsun May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinzuoyan May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinzuoyan May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shengofsun commented May 11, 2018

shengofsun commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neverchanje commented May 9, 2018 •

edited

Loading

shengofsun May 14, 2018 •

edited

Loading

qinzuoyan May 10, 2018 •

edited

Loading

shengofsun May 14, 2018 •

edited

Loading

qinzuoyan May 10, 2018 •

edited

Loading

shengofsun May 14, 2018 •

edited

Loading

qinzuoyan May 10, 2018 •

edited

Loading

qinzuoyan May 15, 2018 •

edited

Loading