Support cancel in AsyncTasks #8581
Conversation
ti-chi-bot
bot
added
release-note-none
|
/run-all-tests |
ti-chi-bot
bot
added
needs-1-more-lgtm
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
|
/run-all-tests |
|
/run-all-tests |
| auto maybe_elapsed = fap_ctx->tasks_trace->queryElapsed(region_id); | ||
| if unlikely (!maybe_elapsed.has_value()) | ||
| { | ||
| GET_METRIC(tiflash_fap_task_result, type_failed_cancel).Increment(); | ||
| LOG_INFO(log, "FAP is canceled at beginning region_id={} new_peer_id={}", region_id, new_peer_id); | ||
| } |
There was a problem hiding this comment.
In what situation will we run into this branch?
There was a problem hiding this comment.
When a cancel happens in addTaskWithCancel between if (cancel_handle->canceled()), and the inner invokable object of (*p)() actually called(after the addTaskWithCancel quitted and release the lock).
It is very slightly chance.
|
/run-unit-test |
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
…improve-async-tasks
| /// `throw_if_noexist` controls whether to throw. | ||
| /// NOTE: The task element will be removed after calling this function. | ||
| template <typename ResultDropper> | ||
| TaskState asyncCancelTask(Key k, ResultDropper result_dropper, bool throw_if_noexist) |
There was a problem hiding this comment.
It could also return std::future<std::optional<R>> to support await-like call pattern. However, we don't make it too complicated. If you want to wait until the worker thread quits, then just use blockedCancelRunningTask
|
/run-all-tests |
| // Only one thread can block cancel and wait. | ||
| RUNTIME_CHECK_MSG(!cancel_handle->canceled(), "Try block cancel running task twice"); |
There was a problem hiding this comment.
why we need this limitation?
There was a problem hiding this comment.
Because the AsyncTasks only supports one consumer, so why would this only consumer cancel twice? I think it must be a mistake.
By checking duplicated key in asyncTasks and checking duplicated cancel in blockedCancelRunningTask will also guard the "one consumer" assumption.
…improve-async-tasks
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
| } | ||
|
|
||
| bool isReady(Key key) const | ||
| TaskState unsafeQueryState(Key key) const |
There was a problem hiding this comment.
this should be private?
There was a problem hiding this comment.
seems the friends decl does not work here too
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
|
/run-all-tests |
| { | ||
| std::unique_lock<std::mutex> l(mtx); | ||
| auto it = tasks.find(key); | ||
| RUNTIME_CHECK(it != tasks.end()); |
There was a problem hiding this comment.
| RUNTIME_CHECK(it != tasks.end()); | |
| RUNTIME_CHECK(it != tasks.end(), key); |
There was a problem hiding this comment.
What if the key is can not be format?
https://en.cppreference.com/w/cpp/utility/format/formattable
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
| void doCancel() | ||
| { | ||
| // Use lock here to prevent losing signal. | ||
| std::scoped_lock<std::mutex> lock(mut); | ||
| inner.store(true); | ||
| cv.notify_all(); | ||
| } |
There was a problem hiding this comment.
nit: Why not acquiring lock will lead to signal lost?
Co-authored-by: JaySon <tshent@qq.com>
|
/hold |
ti-chi-bot
bot
added
the
do-not-merge/hold
…improve-async-tasks
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
|
/run-all-tests |
What problem does this PR solve?
Issue Number: close #8382
Problem Summary:
The
AsyncTasksis a very simple manager for running and fetching result from a non-work-stealing thread pool. It could only have one producer and one consumer per task.In order to support canceling the running or queueing tasks, we proposed this PR.
Added cancel for
AsyncTasks. So the transition will beCallers should guarantee a task is no longer accessible once canceled or has its result fetched.
The
AsyncTasksis designed to have one consumer. If a task is canceled, then it will be unregistered fromAsyncTasksimmediately, the caller has to keep track of the state, and avoid calling cancel or fetching result again for the same task.The
AsyncTaskswill panic if register another task with the same key. However, if the task is canceled asyncly, the worker thread may not find acanceledcheckpoint immediately. If a new task is added in the meantime, then there could be two tasks manipulating some shared resources. In order to avoid this situation, block wait for cancel.What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note