Figure out lifetimes issues with PostTaskAndReply and OnceClosure |
||||
Issue description
PostTaskAndReply is implemented with a helper class. The class is pretty small and looks like this:
class PostTaskAndReplyRelay {
public:
PostTaskAndReplyRelay(const tracked_objects::Location& from_here,
const Closure& task,
const Closure& reply)
: sequence_checker_(),
from_here_(from_here),
origin_task_runner_(SequencedTaskRunnerHandle::Get()),
reply_(reply),
task_(task) {}
~PostTaskAndReplyRelay() {
DCHECK(sequence_checker_.CalledOnValidSequence());
task_.Reset();
reply_.Reset();
}
void RunTaskAndPostReply() {
task_.Run();
origin_task_runner_->PostTask(
from_here_, Bind(&PostTaskAndReplyRelay::RunReplyAndSelfDestruct,
base::Unretained(this)));
}
private:
void RunReplyAndSelfDestruct() {
DCHECK(sequence_checker_.CalledOnValidSequence());
// Force |task_| to be released before |reply_| is to ensure that no one
// accidentally depends on |task_| keeping one of its arguments alive while
// |reply_| is executing.
task_.Reset();
reply_.Run();
// Cue mission impossible theme.
delete this;
}
const SequenceChecker sequence_checker_;
const tracked_objects::Location from_here_;
const scoped_refptr<SequencedTaskRunner> origin_task_runner_;
Closure reply_;
Closure task_;
};
The problem is this class explicitly guarantees that |task_| (and its bound parameters) will be destroyed on |origin_task_runner_|:
// This relay class remembers the sequence that it was created on, and ensures
// that both the |task| and |reply| Closures are deleted on this same sequence.
// Also, |task| is guaranteed to be deleted before |reply| is run or deleted.
However, if we're using OnceCallback, that's no longer possible: the BindState will be consumed on the target task runner, and any bound variables will be destroyed on the target task runner, rather than the origin task runner.
This is a hard blocker for using OnceClosure in more places.
,
Jan 12 2017
Thinking about this more, I think we can just say that |task| must always be a Callback. This lifetime model is hostile to the use case that OnceCallback is trying to address: OnceCallback wants to *always* transfer ownership to the target task runner, but the semantics of this imply exactly the opposite.
,
Jan 31 2017
I looked through all callers of PostTaskAndReply family that I could reach on the code search, and found that there were only a few callers that depends on the fact |task| is destroyed on the original thread. These dependencies were just unintentional ones and I already removed. https://codereview.chromium.org/2655213004/ https://codereview.chromium.org/2657003003/ I believe now it's safe. To be conservative in case I overlook a secret dependency to the destruction thread, we can add a new version of PTAR in another name, touch all caller of the old API to migrate to the new API, and rename it back.
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba commit e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba Author: tzik <tzik@chromium.org> Date: Thu Mar 30 18:05:46 2017 Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. BUG= 680359 Review-Url: https://codereview.chromium.org/2657603004 Cr-Commit-Position: refs/heads/master@{#460821} [modify] https://crrev.com/e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba/base/message_loop/message_loop_task_runner_unittest.cc [modify] https://crrev.com/e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba/base/threading/post_task_and_reply_impl.cc [modify] https://crrev.com/e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba/base/threading/post_task_and_reply_impl_unittest.cc
,
Apr 13 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dcheng@chromium.org
, Jan 12 2017