Reland of Make the default ctor of rtc::Thread, protected This is a partial re-land. The change doesn't make the default Thread ctor protected anymore but it does mark it as deprecated and updates all use of it in WebRTC. Original issue's description: Make the default ctor of rtc::Thread, protected. The goal is to force use of Thread::Create or Thread::CreateWithSocketServer. The default constructor constructs a 'default' socket server, which is usually a 'physical' socket server, but not always. Not every instance of Thread actually needs to have network support, so it's better to have this be explicit instead of unknowingly instantiate one. BUG=none Review-Url: https://codereview.webrtc.org/2977953002 Cr-Commit-Position: refs/heads/master@{#19031}
diff --git a/webrtc/rtc_base/criticalsection_unittest.cc b/webrtc/rtc_base/criticalsection_unittest.cc index 672a674..2e136bf 100644 --- a/webrtc/rtc_base/criticalsection_unittest.cc +++ b/webrtc/rtc_base/criticalsection_unittest.cc
@@ -201,7 +201,7 @@ void StartThreads(std::vector<std::unique_ptr<Thread>>* threads, MessageHandler* handler) { for (int i = 0; i < kNumThreads; ++i) { - std::unique_ptr<Thread> thread(new Thread()); + std::unique_ptr<Thread> thread(Thread::Create()); thread->Start(); thread->Post(RTC_FROM_HERE, handler); threads->push_back(std::move(thread));
diff --git a/webrtc/rtc_base/logging_unittest.cc b/webrtc/rtc_base/logging_unittest.cc index 5e7f67c..c806077 100644 --- a/webrtc/rtc_base/logging_unittest.cc +++ b/webrtc/rtc_base/logging_unittest.cc
@@ -8,9 +8,10 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "webrtc/rtc_base/logging.h" #include "webrtc/rtc_base/fileutils.h" #include "webrtc/rtc_base/gunit.h" +#include "webrtc/rtc_base/logging.h" +#include "webrtc/rtc_base/nullsocketserver.h" #include "webrtc/rtc_base/pathutils.h" #include "webrtc/rtc_base/stream.h" #include "webrtc/rtc_base/thread.h" @@ -88,6 +89,8 @@ // We should restore the correct global state at the end. class LogThread : public Thread { public: + LogThread() : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())) {} + ~LogThread() override { Stop(); }
diff --git a/webrtc/rtc_base/messagequeue_unittest.cc b/webrtc/rtc_base/messagequeue_unittest.cc index e31adf9..4003e2d 100644 --- a/webrtc/rtc_base/messagequeue_unittest.cc +++ b/webrtc/rtc_base/messagequeue_unittest.cc
@@ -38,9 +38,9 @@ bool IsLocked() { // We have to do this on a worker thread, or else the TryEnter will // succeed, since our critical sections are reentrant. - Thread worker; - worker.Start(); - return worker.Invoke<bool>( + std::unique_ptr<Thread> worker(Thread::CreateWithSocketServer()); + worker->Start(); + return worker->Invoke<bool>( RTC_FROM_HERE, rtc::Bind(&MessageQueueTest::IsLocked_Worker, this)); } }; @@ -152,10 +152,10 @@ // all registered message queues. TEST(MessageQueueManager, ProcessAllMessageQueues) { Event entered_process_all_message_queues(true, false); - Thread a; - Thread b; - a.Start(); - b.Start(); + auto a = Thread::CreateWithSocketServer(); + auto b = Thread::CreateWithSocketServer(); + a->Start(); + b->Start(); volatile int messages_processed = 0; FunctorMessageHandler<void, std::function<void()>> incrementer( @@ -173,10 +173,10 @@ }); // Post messages (both delayed and non delayed) to both threads. - a.Post(RTC_FROM_HERE, &incrementer); - b.Post(RTC_FROM_HERE, &incrementer); - a.PostDelayed(RTC_FROM_HERE, 0, &incrementer); - b.PostDelayed(RTC_FROM_HERE, 0, &incrementer); + a->Post(RTC_FROM_HERE, &incrementer); + b->Post(RTC_FROM_HERE, &incrementer); + a->PostDelayed(RTC_FROM_HERE, 0, &incrementer); + b->PostDelayed(RTC_FROM_HERE, 0, &incrementer); rtc::Thread::Current()->Post(RTC_FROM_HERE, &event_signaler); MessageQueueManager::ProcessAllMessageQueues(); @@ -185,9 +185,9 @@ // Test that ProcessAllMessageQueues doesn't hang if a thread is quitting. TEST(MessageQueueManager, ProcessAllMessageQueuesWithQuittingThread) { - Thread t; - t.Start(); - t.Quit(); + auto t = Thread::CreateWithSocketServer(); + t->Start(); + t->Quit(); MessageQueueManager::ProcessAllMessageQueues(); } @@ -195,8 +195,8 @@ // messages. TEST(MessageQueueManager, ProcessAllMessageQueuesWithClearedQueue) { Event entered_process_all_message_queues(true, false); - Thread t; - t.Start(); + auto t = Thread::CreateWithSocketServer(); + t->Start(); FunctorMessageHandler<void, std::function<void()>> clearer( [&entered_process_all_message_queues] { @@ -213,7 +213,7 @@ }); // Post messages (both delayed and non delayed) to both threads. - t.Post(RTC_FROM_HERE, &clearer); + t->Post(RTC_FROM_HERE, &clearer); rtc::Thread::Current()->Post(RTC_FROM_HERE, &event_signaler); MessageQueueManager::ProcessAllMessageQueues(); } @@ -231,7 +231,7 @@ }; TEST(MessageQueueManager, ClearReentrant) { - Thread t; + std::unique_ptr<Thread> t(Thread::Create()); EmptyHandler handler; RefCountedHandler* inner_handler( new rtc::RefCountedObject<RefCountedHandler>()); @@ -242,7 +242,7 @@ // The inner handler will be removed in a re-entrant fashion from the // message queue of the thread while the outer handler is removed, verifying // that the iterator is not invalidated in "MessageQueue::Clear". - t.Post(RTC_FROM_HERE, inner_handler, 0); - t.Post(RTC_FROM_HERE, &handler, 0, - new ScopedRefMessageData<RefCountedHandler>(inner_handler)); + t->Post(RTC_FROM_HERE, inner_handler, 0); + t->Post(RTC_FROM_HERE, &handler, 0, + new ScopedRefMessageData<RefCountedHandler>(inner_handler)); }
diff --git a/webrtc/rtc_base/nullsocketserver_unittest.cc b/webrtc/rtc_base/nullsocketserver_unittest.cc index 5908d32..5538322 100644 --- a/webrtc/rtc_base/nullsocketserver_unittest.cc +++ b/webrtc/rtc_base/nullsocketserver_unittest.cc
@@ -28,9 +28,9 @@ }; TEST_F(NullSocketServerTest, WaitAndSet) { - Thread thread; - EXPECT_TRUE(thread.Start()); - thread.Post(RTC_FROM_HERE, this, 0); + auto thread = Thread::Create(); + EXPECT_TRUE(thread->Start()); + thread->Post(RTC_FROM_HERE, this, 0); // The process_io will be ignored. const bool process_io = true; EXPECT_TRUE_WAIT(ss_.Wait(SocketServer::kForever, process_io), kTimeout);
diff --git a/webrtc/rtc_base/physicalsocketserver_unittest.cc b/webrtc/rtc_base/physicalsocketserver_unittest.cc index ee95c23..34ba027 100644 --- a/webrtc/rtc_base/physicalsocketserver_unittest.cc +++ b/webrtc/rtc_base/physicalsocketserver_unittest.cc
@@ -610,7 +610,7 @@ // Start a new thread that raises it. It will have to be delivered to that // thread. Our implementation should safely handle it and dispatch // RecordSignal() on this thread. - std::unique_ptr<Thread> thread(new Thread()); + std::unique_ptr<Thread> thread(Thread::CreateWithSocketServer()); std::unique_ptr<RaiseSigTermRunnable> runnable(new RaiseSigTermRunnable()); thread->Start(runnable.get()); EXPECT_TRUE(ss_->Wait(1500, true));
diff --git a/webrtc/rtc_base/rtccertificategenerator_unittest.cc b/webrtc/rtc_base/rtccertificategenerator_unittest.cc index f09f797..df820d9 100644 --- a/webrtc/rtc_base/rtccertificategenerator_unittest.cc +++ b/webrtc/rtc_base/rtccertificategenerator_unittest.cc
@@ -24,7 +24,7 @@ public: RTCCertificateGeneratorFixture() : signaling_thread_(Thread::Current()), - worker_thread_(new Thread()), + worker_thread_(Thread::Create()), generate_async_completed_(false) { RTC_CHECK(signaling_thread_); RTC_CHECK(worker_thread_->Start());
diff --git a/webrtc/rtc_base/socket_unittest.cc b/webrtc/rtc_base/socket_unittest.cc index 35bf2d1..5ef0167 100644 --- a/webrtc/rtc_base/socket_unittest.cc +++ b/webrtc/rtc_base/socket_unittest.cc
@@ -681,7 +681,7 @@ EXPECT_FALSE(sink.Check(accepted.get(), SSE_READ)); // Shouldn't signal when blocked in a thread Send, where process_io is false. - std::unique_ptr<Thread> thread(new Thread()); + std::unique_ptr<Thread> thread(Thread::CreateWithSocketServer()); thread->Start(); Sleeper sleeper; TypedMessageData<AsyncSocket*> data(client.get());
diff --git a/webrtc/rtc_base/thread.cc b/webrtc/rtc_base/thread.cc index 56a145c..9ba0f51 100644 --- a/webrtc/rtc_base/thread.cc +++ b/webrtc/rtc_base/thread.cc
@@ -44,7 +44,7 @@ #ifndef NO_MAIN_THREAD_WRAPPING // Only autowrap the thread which instantiated the ThreadManager. if (!thread && manager->IsMainThread()) { - thread = new Thread(); + thread = new Thread(SocketServer::CreateDefault()); thread->WrapCurrentWithThreadManager(manager, true); } #endif @@ -87,7 +87,7 @@ Thread *ThreadManager::WrapCurrentThread() { Thread* result = CurrentThread(); if (nullptr == result) { - result = new Thread(); + result = new Thread(SocketServer::CreateDefault()); result->WrapCurrentWithThreadManager(this, true); } return result; @@ -115,6 +115,7 @@ thread_->SetAllowBlockingCalls(previous_state_); } +// DEPRECATED. Thread::Thread() : Thread(SocketServer::CreateDefault()) {} Thread::Thread(SocketServer* ss) @@ -520,7 +521,7 @@ return true; } -AutoThread::AutoThread() { +AutoThread::AutoThread() : Thread(SocketServer::CreateDefault()) { if (!ThreadManager::Instance()->CurrentThread()) { ThreadManager::Instance()->SetCurrentThread(this); }
diff --git a/webrtc/rtc_base/thread.h b/webrtc/rtc_base/thread.h index 35b28ea..f037d8a 100644 --- a/webrtc/rtc_base/thread.h +++ b/webrtc/rtc_base/thread.h
@@ -102,8 +102,13 @@ class LOCKABLE Thread : public MessageQueue { public: - // Create a new Thread and optionally assign it to the passed SocketServer. + // DEPRECATED. + // The default constructor should not be used because it hides whether or + // not a socket server will be associated with the thread. Most instances + // of Thread do actually not need one, so please use either of the Create* + // methods to construct an instance of Thread. Thread(); + explicit Thread(SocketServer* ss); explicit Thread(std::unique_ptr<SocketServer> ss);
diff --git a/webrtc/rtc_base/thread_checker_unittest.cc b/webrtc/rtc_base/thread_checker_unittest.cc index 42e1fcc..d8ad830 100644 --- a/webrtc/rtc_base/thread_checker_unittest.cc +++ b/webrtc/rtc_base/thread_checker_unittest.cc
@@ -14,6 +14,7 @@ #include "webrtc/rtc_base/checks.h" #include "webrtc/rtc_base/constructormagic.h" +#include "webrtc/rtc_base/nullsocketserver.h" #include "webrtc/rtc_base/task_queue.h" #include "webrtc/rtc_base/thread.h" #include "webrtc/rtc_base/thread_checker.h" @@ -52,7 +53,7 @@ class CallDoStuffOnThread : public Thread { public: explicit CallDoStuffOnThread(ThreadCheckerClass* thread_checker_class) - : Thread(), + : Thread(std::unique_ptr<SocketServer>(new rtc::NullSocketServer())), thread_checker_class_(thread_checker_class) { SetName("call_do_stuff_on_thread", nullptr); } @@ -75,9 +76,9 @@ class DeleteThreadCheckerClassOnThread : public Thread { public: explicit DeleteThreadCheckerClassOnThread( - ThreadCheckerClass* thread_checker_class) - : Thread(), - thread_checker_class_(thread_checker_class) { + std::unique_ptr<ThreadCheckerClass> thread_checker_class) + : Thread(std::unique_ptr<SocketServer>(new rtc::NullSocketServer())), + thread_checker_class_(std::move(thread_checker_class)) { SetName("delete_thread_checker_class_on_thread", nullptr); } @@ -89,6 +90,8 @@ Thread::Join(); } + bool has_been_deleted() const { return !thread_checker_class_; } + private: std::unique_ptr<ThreadCheckerClass> thread_checker_class_; @@ -115,10 +118,14 @@ // Verify that the destructor doesn't assert // when called on a different thread. DeleteThreadCheckerClassOnThread delete_on_thread( - thread_checker_class.release()); + std::move(thread_checker_class)); + + EXPECT_FALSE(delete_on_thread.has_been_deleted()); delete_on_thread.Start(); delete_on_thread.Join(); + + EXPECT_TRUE(delete_on_thread.has_been_deleted()); } TEST(ThreadCheckerTest, DetachFromThread) {
diff --git a/webrtc/rtc_base/thread_unittest.cc b/webrtc/rtc_base/thread_unittest.cc index db7d038..a8c20d1 100644 --- a/webrtc/rtc_base/thread_unittest.cc +++ b/webrtc/rtc_base/thread_unittest.cc
@@ -14,6 +14,7 @@ #include "webrtc/rtc_base/asyncudpsocket.h" #include "webrtc/rtc_base/event.h" #include "webrtc/rtc_base/gunit.h" +#include "webrtc/rtc_base/nullsocketserver.h" #include "webrtc/rtc_base/physicalsocketserver.h" #include "webrtc/rtc_base/sigslot.h" #include "webrtc/rtc_base/socketaddress.h" @@ -106,7 +107,8 @@ class CustomThread : public rtc::Thread { public: - CustomThread() {} + CustomThread() + : Thread(std::unique_ptr<SocketServer>(new rtc::NullSocketServer())) {} virtual ~CustomThread() { Stop(); } bool Start() { return false; } @@ -124,8 +126,8 @@ class SignalWhenDestroyedThread : public Thread { public: SignalWhenDestroyedThread(Event* event) - : event_(event) { - } + : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())), + event_(event) {} virtual ~SignalWhenDestroyedThread() { Stop(); @@ -195,24 +197,24 @@ const SocketAddress addr("127.0.0.1", 0); // Create the messaging client on its own thread. - Thread th1; - Socket* socket = th1.socketserver()->CreateAsyncSocket(addr.family(), - SOCK_DGRAM); - MessageClient msg_client(&th1, socket); + auto th1 = Thread::CreateWithSocketServer(); + Socket* socket = + th1->socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM); + MessageClient msg_client(th1.get(), socket); // Create the socket client on its own thread. - Thread th2; + auto th2 = Thread::CreateWithSocketServer(); AsyncSocket* asocket = - th2.socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM); - SocketClient sock_client(asocket, addr, &th1, &msg_client); + th2->socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM); + SocketClient sock_client(asocket, addr, th1.get(), &msg_client); socket->Connect(sock_client.address()); - th1.Start(); - th2.Start(); + th1->Start(); + th2->Start(); // Get the messages started. - th1.PostDelayed(RTC_FROM_HERE, 100, &msg_client, 0, new TestMessage(1)); + th1->PostDelayed(RTC_FROM_HERE, 100, &msg_client, 0, new TestMessage(1)); // Give the clients a little while to run. // Messages will be processed at 100, 300, 500, 700, 900. @@ -221,9 +223,9 @@ // Stop the sending client. Give the receiver a bit longer to run, in case // it is running on a machine that is under load (e.g. the build machine). - th1.Stop(); + th1->Stop(); th_main->ProcessMessages(200); - th2.Stop(); + th2->Stop(); // Make sure the results were correct EXPECT_EQ(5, msg_client.count); @@ -236,23 +238,19 @@ // There's no easy way to verify the name was set properly at this time. TEST(ThreadTest, Names) { // Default name - Thread *thread; - thread = new Thread(); + auto thread = Thread::CreateWithSocketServer(); EXPECT_TRUE(thread->Start()); thread->Stop(); - delete thread; - thread = new Thread(); // Name with no object parameter + thread = Thread::CreateWithSocketServer(); EXPECT_TRUE(thread->SetName("No object", nullptr)); EXPECT_TRUE(thread->Start()); thread->Stop(); - delete thread; // Really long name - thread = new Thread(); + thread = Thread::CreateWithSocketServer(); EXPECT_TRUE(thread->SetName("Abcdefghijklmnopqrstuvwxyz1234567890", this)); EXPECT_TRUE(thread->Start()); thread->Stop(); - delete thread; } TEST(ThreadTest, Wrap) { @@ -270,21 +268,21 @@ TEST(ThreadTest, Invoke) { // Create and start the thread. - Thread thread; - thread.Start(); + auto thread = Thread::CreateWithSocketServer(); + thread->Start(); // Try calling functors. - EXPECT_EQ(42, thread.Invoke<int>(RTC_FROM_HERE, FunctorA())); + EXPECT_EQ(42, thread->Invoke<int>(RTC_FROM_HERE, FunctorA())); AtomicBool called; FunctorB f2(&called); - thread.Invoke<void>(RTC_FROM_HERE, f2); + thread->Invoke<void>(RTC_FROM_HERE, f2); EXPECT_TRUE(called.get()); // Try calling bare functions. struct LocalFuncs { static int Func1() { return 999; } static void Func2() {} }; - EXPECT_EQ(999, thread.Invoke<int>(RTC_FROM_HERE, &LocalFuncs::Func1)); - thread.Invoke<void>(RTC_FROM_HERE, &LocalFuncs::Func2); + EXPECT_EQ(999, thread->Invoke<int>(RTC_FROM_HERE, &LocalFuncs::Func1)); + thread->Invoke<void>(RTC_FROM_HERE, &LocalFuncs::Func2); } // Verifies that two threads calling Invoke on each other at the same time does @@ -294,8 +292,8 @@ Thread* current_thread = Thread::Current(); ASSERT_TRUE(current_thread != nullptr); - Thread other_thread; - other_thread.Start(); + auto other_thread = Thread::CreateWithSocketServer(); + other_thread->Start(); struct LocalFuncs { static void Set(bool* out) { *out = true; } @@ -305,7 +303,7 @@ }; bool called = false; - other_thread.Invoke<void>( + other_thread->Invoke<void>( RTC_FROM_HERE, Bind(&LocalFuncs::InvokeSet, current_thread, &called)); EXPECT_TRUE(called); @@ -317,9 +315,10 @@ TEST(ThreadTest, ThreeThreadsInvoke) { AutoThread thread; Thread* thread_a = Thread::Current(); - Thread thread_b, thread_c; - thread_b.Start(); - thread_c.Start(); + auto thread_b = Thread::CreateWithSocketServer(); + auto thread_c = Thread::CreateWithSocketServer(); + thread_b->Start(); + thread_c->Start(); class LockedBool { public: @@ -377,9 +376,9 @@ // Start the sequence A --(invoke)--> B --(async invoke)--> C --(invoke)--> A. // Thread B returns when C receives the call and C should be blocked until A // starts to process messages. - thread_b.Invoke<void>(RTC_FROM_HERE, - Bind(&LocalFuncs::AsyncInvokeSetAndWait, &invoker, - &thread_c, thread_a, &thread_a_called)); + thread_b->Invoke<void>(RTC_FROM_HERE, + Bind(&LocalFuncs::AsyncInvokeSetAndWait, &invoker, + thread_c.get(), thread_a, &thread_a_called)); EXPECT_FALSE(thread_a_called.Get()); EXPECT_TRUE_WAIT(thread_a_called.Get(), 2000); @@ -406,9 +405,9 @@ }; TEST(ThreadTest, SetNameOnSignalQueueDestroyed) { - Thread* thread1 = new Thread(); - SetNameOnSignalQueueDestroyedTester tester1(thread1); - delete thread1; + auto thread1 = Thread::CreateWithSocketServer(); + SetNameOnSignalQueueDestroyedTester tester1(thread1.get()); + thread1.reset(); Thread* thread2 = new AutoThread(); SetNameOnSignalQueueDestroyedTester tester2(thread2); @@ -438,12 +437,13 @@ TEST_F(AsyncInvokeTest, FireAndForget) { AsyncInvoker invoker; // Create and start the thread. - Thread thread; - thread.Start(); + auto thread = Thread::CreateWithSocketServer(); + thread->Start(); // Try calling functor. AtomicBool called; - invoker.AsyncInvoke<void>(RTC_FROM_HERE, &thread, FunctorB(&called)); + invoker.AsyncInvoke<void>(RTC_FROM_HERE, thread.get(), FunctorB(&called)); EXPECT_TRUE_WAIT(called.get(), kWaitTimeout); + thread->Stop(); } TEST_F(AsyncInvokeTest, KillInvokerDuringExecute) { @@ -454,12 +454,12 @@ Event functor_continue(false, false); Event functor_finished(false, false); - Thread thread; - thread.Start(); + auto thread = Thread::CreateWithSocketServer(); + thread->Start(); volatile bool invoker_destroyed = false; { AsyncInvoker invoker; - invoker.AsyncInvoke<void>(RTC_FROM_HERE, &thread, + invoker.AsyncInvoke<void>(RTC_FROM_HERE, thread.get(), [&functor_started, &functor_continue, &functor_finished, &invoker_destroyed] { functor_started.Set(); @@ -550,7 +550,7 @@ // Test that we can call AsyncInvoke<void>() after the thread died. TEST_F(GuardedAsyncInvokeTest, KillThreadFireAndForget) { // Create and start the thread. - std::unique_ptr<Thread> thread(new Thread()); + std::unique_ptr<Thread> thread(Thread::Create()); thread->Start(); std::unique_ptr<GuardedAsyncInvoker> invoker; // Create the invoker on |thread|.
diff --git a/webrtc/rtc_base/timeutils_unittest.cc b/webrtc/rtc_base/timeutils_unittest.cc index 5fd9436..a409fb6 100644 --- a/webrtc/rtc_base/timeutils_unittest.cc +++ b/webrtc/rtc_base/timeutils_unittest.cc
@@ -349,8 +349,8 @@ FakeClock clock; SetClockForTesting(&clock); - Thread worker; - worker.Start(); + std::unique_ptr<Thread> worker(Thread::CreateWithSocketServer()); + worker->Start(); // Post an event that won't be executed for 10 seconds. Event message_handler_dispatched(false, false); @@ -358,7 +358,7 @@ message_handler_dispatched.Set(); }; FunctorMessageHandler<void, decltype(functor)> handler(functor); - worker.PostDelayed(RTC_FROM_HERE, 60000, &handler); + worker->PostDelayed(RTC_FROM_HERE, 60000, &handler); // Wait for a bit for the worker thread to be started and enter its socket // select(). Otherwise this test would be trivial since the worker thread @@ -369,7 +369,7 @@ // and dispatch the message instantly. clock.AdvanceTime(TimeDelta::FromSeconds(60u)); EXPECT_TRUE(message_handler_dispatched.Wait(0)); - worker.Stop(); + worker->Stop(); SetClockForTesting(nullptr);