diff --git a/rtc_base/task_queue.h b/rtc_base/task_queue.h index fe4ad5f4c0..7842ea3c2b 100644 --- a/rtc_base/task_queue.h +++ b/rtc_base/task_queue.h @@ -15,9 +15,11 @@ #include #include #include +#include #include "rtc_base/constructormagic.h" #include "rtc_base/criticalsection.h" +#include "rtc_base/ptr_util.h" #include "rtc_base/scoped_ref_ptr.h" namespace rtc { @@ -45,7 +47,8 @@ class QueuedTask { template class ClosureTask : public QueuedTask { public: - explicit ClosureTask(const Closure& closure) : closure_(closure) {} + explicit ClosureTask(Closure&& closure) + : closure_(std::forward(closure)) {} private: bool Run() override { @@ -53,7 +56,8 @@ class ClosureTask : public QueuedTask { return true; } - Closure closure_; + typename std::remove_const< + typename std::remove_reference::type>::type closure_; }; // Extends ClosureTask to also allow specifying cleanup code. @@ -62,27 +66,29 @@ class ClosureTask : public QueuedTask { template class ClosureTaskWithCleanup : public ClosureTask { public: - ClosureTaskWithCleanup(const Closure& closure, Cleanup cleanup) - : ClosureTask(closure), cleanup_(cleanup) {} + ClosureTaskWithCleanup(Closure&& closure, Cleanup&& cleanup) + : ClosureTask(std::forward(closure)), + cleanup_(std::forward(cleanup)) {} ~ClosureTaskWithCleanup() { cleanup_(); } private: - Cleanup cleanup_; + typename std::remove_const< + typename std::remove_reference::type>::type cleanup_; }; // Convenience function to construct closures that can be passed directly // to methods that support std::unique_ptr but not template // based parameters. template -static std::unique_ptr NewClosure(const Closure& closure) { - return std::unique_ptr(new ClosureTask(closure)); +static std::unique_ptr NewClosure(Closure&& closure) { + return rtc::MakeUnique>(std::forward(closure)); } template -static std::unique_ptr NewClosure(const Closure& closure, - const Cleanup& cleanup) { - return std::unique_ptr( - new ClosureTaskWithCleanup(closure, cleanup)); +static std::unique_ptr NewClosure(Closure&& closure, + Cleanup&& cleanup) { + return rtc::MakeUnique>( + std::forward(closure), std::forward(cleanup)); } // Implements a task queue that asynchronously executes tasks in a way that @@ -185,50 +191,44 @@ class RTC_LOCKABLE TaskQueue { // std::unique_ptr would not end up being // caught by this template. template ::value>::type* = nullptr> - void PostTask(const Closure& closure) { - PostTask(std::unique_ptr(new ClosureTask(closure))); + typename std::enable_if>::value>::type* = nullptr> + void PostTask(Closure&& closure) { + PostTask(NewClosure(std::forward(closure))); } // See documentation above for performance expectations. - template - void PostDelayedTask(const Closure& closure, uint32_t milliseconds) { - PostDelayedTask( - std::unique_ptr(new ClosureTask(closure)), - milliseconds); + template >::value>::type* = nullptr> + void PostDelayedTask(Closure&& closure, uint32_t milliseconds) { + PostDelayedTask(NewClosure(std::forward(closure)), milliseconds); } template - void PostTaskAndReply(const Closure1& task, - const Closure2& reply, + void PostTaskAndReply(Closure1&& task, + Closure2&& reply, TaskQueue* reply_queue) { - PostTaskAndReply( - std::unique_ptr(new ClosureTask(task)), - std::unique_ptr(new ClosureTask(reply)), - reply_queue); + PostTaskAndReply(NewClosure(std::forward(task)), + NewClosure(std::forward(reply)), reply_queue); } template - void PostTaskAndReply(std::unique_ptr task, - const Closure& reply) { - PostTaskAndReply(std::move(task), std::unique_ptr( - new ClosureTask(reply))); + void PostTaskAndReply(std::unique_ptr task, Closure&& reply) { + PostTaskAndReply(std::move(task), NewClosure(std::forward(reply))); } template - void PostTaskAndReply(const Closure& task, - std::unique_ptr reply) { - PostTaskAndReply( - std::unique_ptr(new ClosureTask(task)), - std::move(reply)); + void PostTaskAndReply(Closure&& task, std::unique_ptr reply) { + PostTaskAndReply(NewClosure(std::forward(task)), std::move(reply)); } template - void PostTaskAndReply(const Closure1& task, const Closure2& reply) { - PostTaskAndReply( - std::unique_ptr(new ClosureTask(task)), - std::unique_ptr(new ClosureTask(reply))); + void PostTaskAndReply(Closure1&& task, Closure2&& reply) { + PostTaskAndReply(NewClosure(std::forward(task)), + NewClosure(std::forward(reply))); } private: diff --git a/rtc_base/task_queue_unittest.cc b/rtc_base/task_queue_unittest.cc index adc43c81af..d70a5fb090 100644 --- a/rtc_base/task_queue_unittest.cc +++ b/rtc_base/task_queue_unittest.cc @@ -239,7 +239,7 @@ TEST(TaskQueueTest, PostAndReuse) { Event* const event_; }; - std::unique_ptr task( + std::unique_ptr task( new ReusedTask(&call_count, &reply_queue, &event)); post_queue.PostTask(std::move(task)); @@ -260,6 +260,105 @@ TEST(TaskQueueTest, PostAndReplyLambda) { EXPECT_TRUE(my_flag); } +TEST(TaskQueueTest, PostCopyableClosure) { + struct CopyableClosure { + CopyableClosure(int* num_copies, int* num_moves, Event* event) + : num_copies(num_copies), num_moves(num_moves), event(event) {} + CopyableClosure(const CopyableClosure& other) + : num_copies(other.num_copies), + num_moves(other.num_moves), + event(other.event) { + ++*num_copies; + } + CopyableClosure(CopyableClosure&& other) + : num_copies(other.num_copies), + num_moves(other.num_moves), + event(other.event) { + ++*num_moves; + } + void operator()() { event->Set(); } + + int* num_copies; + int* num_moves; + Event* event; + }; + + int num_copies = 0; + int num_moves = 0; + Event event(false, false); + + static const char kPostQueue[] = "PostCopyableClosure"; + TaskQueue post_queue(kPostQueue); + { + CopyableClosure closure(&num_copies, &num_moves, &event); + post_queue.PostTask(closure); + // Destroy closure to check with msan and tsan posted task has own copy. + } + + EXPECT_TRUE(event.Wait(1000)); + EXPECT_EQ(num_copies, 1); + EXPECT_EQ(num_moves, 0); +} + +TEST(TaskQueueTest, PostMoveOnlyClosure) { + struct SomeState { + explicit SomeState(Event* event) : event(event) {} + ~SomeState() { event->Set(); } + Event* event; + }; + struct MoveOnlyClosure { + MoveOnlyClosure(int* num_moves, std::unique_ptr state) + : num_moves(num_moves), state(std::move(state)) {} + MoveOnlyClosure(const MoveOnlyClosure&) = delete; + MoveOnlyClosure(MoveOnlyClosure&& other) + : num_moves(other.num_moves), state(std::move(other.state)) { + ++*num_moves; + } + void operator()() { state.reset(); } + + int* num_moves; + std::unique_ptr state; + }; + + int num_moves = 0; + Event event(false, false); + std::unique_ptr state(new SomeState(&event)); + + static const char kPostQueue[] = "PostMoveOnlyClosure"; + TaskQueue post_queue(kPostQueue); + post_queue.PostTask(MoveOnlyClosure(&num_moves, std::move(state))); + + EXPECT_TRUE(event.Wait(1000)); + EXPECT_EQ(num_moves, 1); +} + +TEST(TaskQueueTest, PostMoveOnlyCleanup) { + struct SomeState { + explicit SomeState(Event* event) : event(event) {} + ~SomeState() { event->Set(); } + Event* event; + }; + struct MoveOnlyClosure { + void operator()() { state.reset(); } + + std::unique_ptr state; + }; + + Event event_run(false, false); + Event event_cleanup(false, false); + std::unique_ptr state_run(new SomeState(&event_run)); + std::unique_ptr state_cleanup(new SomeState(&event_cleanup)); + + static const char kPostQueue[] = "PostMoveOnlyCleanup"; + TaskQueue post_queue(kPostQueue); + post_queue.PostTask(NewClosure(MoveOnlyClosure{std::move(state_run)}, + MoveOnlyClosure{std::move(state_cleanup)})); + + EXPECT_TRUE(event_cleanup.Wait(1000)); + // Expect run closure to complete before cleanup closure. + EXPECT_TRUE(event_run.Wait(0)); +} + // This test covers a particular bug that we had in the libevent implementation // where we could hit a deadlock while trying to post a reply task to a queue // that was being deleted. The test isn't guaranteed to hit that case but it's diff --git a/rtc_base/task_queue_win.cc b/rtc_base/task_queue_win.cc index 1d069e6040..082bf9d631 100644 --- a/rtc_base/task_queue_win.cc +++ b/rtc_base/task_queue_win.cc @@ -15,6 +15,7 @@ #include #include +#include #include "rtc_base/arraysize.h" #include "rtc_base/checks.h" @@ -170,10 +171,11 @@ class TaskQueue::Impl : public RefCountInterface { bool IsCurrent() const; template ::value>::type* = nullptr> - void PostTask(const Closure& closure) { - PostTask(std::unique_ptr(new ClosureTask(closure))); + typename std::enable_if>::value>::type* = nullptr> + void PostTask(Closure&& closure) { + PostTask(NewClosure(std::forward(closure))); } void PostTask(std::unique_ptr task);