diff --git a/talk/media/webrtc/webrtcvoiceengine_unittest.cc b/talk/media/webrtc/webrtcvoiceengine_unittest.cc index cb91723dbc..434548a918 100644 --- a/talk/media/webrtc/webrtcvoiceengine_unittest.cc +++ b/talk/media/webrtc/webrtcvoiceengine_unittest.cc @@ -3185,25 +3185,6 @@ TEST(WebRtcVoiceEngineTest, DISABLED_HasUnencryptedLogging) { EXPECT_TRUE(cleartext); } -// Tests we do not see any references to a monitor thread being spun up -// when initiating the engine. -TEST(WebRtcVoiceEngineTest, HasNoMonitorThread) { - cricket::WebRtcVoiceEngine engine; - rtc::scoped_ptr stream( - new rtc::MemoryStream); - rtc::LogMessage::AddLogToStream(stream.get(), rtc::LS_VERBOSE); - engine.SetLogging(rtc::LS_VERBOSE, ""); - EXPECT_TRUE(engine.Init(rtc::Thread::Current())); - engine.Terminate(); - rtc::LogMessage::RemoveLogToStream(stream.get()); - - size_t size = 0; - EXPECT_TRUE(stream->GetSize(&size)); - EXPECT_GT(size, 0U); - const std::string logs(stream->GetBuffer(), size); - EXPECT_NE(std::string::npos, logs.find("ProcessThread")); -} - // Tests that the library is configured with the codecs we want. TEST(WebRtcVoiceEngineTest, HasCorrectCodecs) { cricket::WebRtcVoiceEngine engine; diff --git a/webrtc/system_wrappers/interface/thread_wrapper.h b/webrtc/system_wrappers/interface/thread_wrapper.h index c2c3431f8a..e8be166ff0 100644 --- a/webrtc/system_wrappers/interface/thread_wrapper.h +++ b/webrtc/system_wrappers/interface/thread_wrapper.h @@ -23,12 +23,13 @@ namespace webrtc { // Object that will be passed by the spawned thread when it enters the callback // function. +// TODO(tommi): Remove this define. #define ThreadObj void* // Callback function that the spawned thread will enter once spawned. // A return value of false is interpreted as that the function has no // more work to do and that the thread can be released. -typedef bool(*ThreadRunFunction)(ThreadObj); +typedef bool(*ThreadRunFunction)(void*); enum ThreadPriority { kLowPriority = 1, @@ -38,11 +39,14 @@ enum ThreadPriority { kRealtimePriority = 5 }; +// Represents a simple worker thread. The implementation must be assumed +// to be single threaded, meaning that all methods of the class, must be +// called from the same thread, including instantiation. class ThreadWrapper { public: enum {kThreadMaxNameLength = 64}; - virtual ~ThreadWrapper() {}; + virtual ~ThreadWrapper() {} // Factory method. Constructor disabled. // @@ -52,24 +56,29 @@ class ThreadWrapper { // prio Thread priority. May require root/admin rights. // thread_name NULL terminated thread name, will be visable in the Windows // debugger. + // TODO(tommi): Remove the priority argument and provide a setter instead. + // TODO(tommi): Make thread_name non-optional (i.e. no default value). static ThreadWrapper* CreateThread(ThreadRunFunction func, - ThreadObj obj, + void* obj, ThreadPriority prio = kNormalPriority, const char* thread_name = 0); - // Get the current thread's kernel thread ID. + // Get the current thread's thread ID. + // NOTE: This is a static method. It returns the id of the calling thread, + // *not* the id of the worker thread that a ThreadWrapper instance represents. + // TODO(tommi): Move outside of the ThreadWrapper class to avoid confusion. static uint32_t GetThreadId(); // Non blocking termination of the spawned thread. Note that it is not safe // to delete this class until the spawned thread has been reclaimed. + // TODO(tommi): Remove this method. virtual void SetNotAlive() = 0; // Tries to spawns a thread and returns true if that was successful. // Additionally, it tries to set thread priority according to the priority // from when CreateThread was called. However, failure to set priority will // not result in a false return value. - // TODO(henrike): add a function for polling whether priority was set or - // not. + // TODO(tommi): Remove the id parameter. virtual bool Start(unsigned int& id) = 0; // Stops the spawned thread and waits for it to be reclaimed with a timeout diff --git a/webrtc/system_wrappers/source/thread_win.cc b/webrtc/system_wrappers/source/thread_win.cc index 2037a6f5ea..0fbf88bc60 100644 --- a/webrtc/system_wrappers/source/thread_win.cc +++ b/webrtc/system_wrappers/source/thread_win.cc @@ -10,11 +10,11 @@ #include "webrtc/system_wrappers/source/thread_win.h" -#include #include #include #include +#include "webrtc/base/checks.h" #include "webrtc/system_wrappers/interface/trace.h" #include "webrtc/system_wrappers/source/set_thread_name_win.h" @@ -22,162 +22,97 @@ namespace webrtc { ThreadWindows::ThreadWindows(ThreadRunFunction func, ThreadObj obj, ThreadPriority prio, const char* thread_name) - : ThreadWrapper(), - run_function_(func), + : run_function_(func), obj_(obj), - alive_(false), - dead_(true), - do_not_close_handle_(false), prio_(prio), - event_(NULL), + event_(CreateEvent(NULL, FALSE, FALSE, NULL)), thread_(NULL), - id_(0), - name_(), - set_thread_name_(false) { - event_ = EventWrapper::Create(); - critsect_stop_ = CriticalSectionWrapper::CreateCriticalSection(); - if (thread_name != NULL) { - // Set the thread name to appear in the VS debugger. - set_thread_name_ = true; - strncpy(name_, thread_name, kThreadMaxNameLength); - } + name_(thread_name ? thread_name : "webrtc") { + DCHECK(func); + DCHECK(event_); } ThreadWindows::~ThreadWindows() { -#ifdef _DEBUG - assert(!alive_); -#endif - if (thread_) { - CloseHandle(thread_); - } - if (event_) { - delete event_; - } - if (critsect_stop_) { - delete critsect_stop_; - } + DCHECK(main_thread_.CalledOnValidThread()); + DCHECK(!thread_); + CloseHandle(event_); } +// static uint32_t ThreadWrapper::GetThreadId() { return GetCurrentThreadId(); } -unsigned int WINAPI ThreadWindows::StartThread(LPVOID lp_parameter) { - static_cast(lp_parameter)->Run(); +// static +DWORD WINAPI ThreadWindows::StartThread(void* param) { + static_cast(param)->Run(); return 0; } -bool ThreadWindows::Start(unsigned int& thread_id) { - if (!run_function_) { +bool ThreadWindows::Start(unsigned int& id) { + DCHECK(main_thread_.CalledOnValidThread()); + DCHECK(!thread_); + + // See bug 2902 for stack size. + DWORD thread_id; + thread_ = ::CreateThread(NULL, 0, &StartThread, this, + STACK_SIZE_PARAM_IS_A_RESERVATION, &thread_id); + if (!thread_ ) { + DCHECK(false) << "CreateThread failed"; return false; } - do_not_close_handle_ = false; - // Set stack size to 1M - thread_ = (HANDLE)_beginthreadex(NULL, 1024 * 1024, StartThread, (void*)this, - 0, &thread_id); - if (thread_ == NULL) { - return false; + id = thread_id; + + if (prio_ != kNormalPriority) { + int priority = THREAD_PRIORITY_NORMAL; + switch (prio_) { + case kLowPriority: + priority = THREAD_PRIORITY_BELOW_NORMAL; + break; + case kHighPriority: + priority = THREAD_PRIORITY_ABOVE_NORMAL; + break; + case kHighestPriority: + priority = THREAD_PRIORITY_HIGHEST; + break; + case kRealtimePriority: + priority = THREAD_PRIORITY_TIME_CRITICAL; + break; + default: + break; + } + + SetThreadPriority(thread_, priority); } - id_ = thread_id; - event_->Wait(INFINITE); - switch (prio_) { - case kLowPriority: - SetThreadPriority(thread_, THREAD_PRIORITY_BELOW_NORMAL); - break; - case kNormalPriority: - SetThreadPriority(thread_, THREAD_PRIORITY_NORMAL); - break; - case kHighPriority: - SetThreadPriority(thread_, THREAD_PRIORITY_ABOVE_NORMAL); - break; - case kHighestPriority: - SetThreadPriority(thread_, THREAD_PRIORITY_HIGHEST); - break; - case kRealtimePriority: - SetThreadPriority(thread_, THREAD_PRIORITY_TIME_CRITICAL); - break; - }; return true; } void ThreadWindows::SetNotAlive() { - alive_ = false; + DCHECK(main_thread_.CalledOnValidThread()); } bool ThreadWindows::Stop() { - critsect_stop_->Enter(); - - // Prevents the handle from being closed in ThreadWindows::Run() - do_not_close_handle_ = true; - alive_ = false; - bool signaled = false; - if (thread_ && !dead_) { - critsect_stop_->Leave(); - - // Wait up to 2 seconds for the thread to complete. - if (WAIT_OBJECT_0 == WaitForSingleObject(thread_, 2000)) { - signaled = true; - } - critsect_stop_->Enter(); - } + DCHECK(main_thread_.CalledOnValidThread()); if (thread_) { + SetEvent(event_); + WaitForSingleObject(thread_, INFINITE); CloseHandle(thread_); - thread_ = NULL; + thread_ = nullptr; } - critsect_stop_->Leave(); - if (dead_ || signaled) { - return true; - } else { - return false; - } + return true; } void ThreadWindows::Run() { - alive_ = true; - dead_ = false; - event_->Set(); - - // All tracing must be after event_->Set to avoid deadlock in Trace. - if (set_thread_name_) { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, id_, - "Thread with name:%s started ", name_); - SetThreadName(static_cast(-1), name_); // -1 == caller thread. - } else { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, id_, - "Thread without name started"); - } + if (!name_.empty()) + SetThreadName(static_cast(-1), name_.c_str()); do { - if (run_function_) { - if (!run_function_(obj_)) { - alive_ = false; - } - } else { - alive_ = false; - } - } while (alive_); - - if (set_thread_name_) { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, id_, - "Thread with name:%s stopped", name_); - } else { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, id_, - "Thread without name stopped"); - } - - critsect_stop_->Enter(); - - if (thread_ && !do_not_close_handle_) { - HANDLE thread = thread_; - thread_ = NULL; - CloseHandle(thread); - } - dead_ = true; - - critsect_stop_->Leave(); -}; + if (!run_function_(obj_)) + break; + } while (WaitForSingleObject(event_, 0) == WAIT_TIMEOUT); +} } // namespace webrtc diff --git a/webrtc/system_wrappers/source/thread_win.h b/webrtc/system_wrappers/source/thread_win.h index 6fb7d3163f..2d8fa92260 100644 --- a/webrtc/system_wrappers/source/thread_win.h +++ b/webrtc/system_wrappers/source/thread_win.h @@ -15,47 +15,34 @@ #include -#include "webrtc/system_wrappers/interface/critical_section_wrapper.h" -#include "webrtc/system_wrappers/interface/event_wrapper.h" +#include "webrtc/base/thread_checker.h" namespace webrtc { class ThreadWindows : public ThreadWrapper { public: - ThreadWindows(ThreadRunFunction func, ThreadObj obj, ThreadPriority prio, + ThreadWindows(ThreadRunFunction func, void* obj, ThreadPriority prio, const char* thread_name); - virtual ~ThreadWindows(); + ~ThreadWindows() override; - virtual bool Start(unsigned int& id); - virtual bool Stop(); - virtual void SetNotAlive(); - - static unsigned int WINAPI StartThread(LPVOID lp_parameter); + bool Start(unsigned int& id) override; + bool Stop() override; + void SetNotAlive() override; protected: - virtual void Run(); + void Run(); private: - ThreadRunFunction run_function_; - ThreadObj obj_; - - bool alive_; - bool dead_; - - // TODO(hellner) - // do_not_close_handle_ member seem pretty redundant. Should be able to remove - // it. Basically it should be fine to reclaim the handle when calling stop - // and in the destructor. - bool do_not_close_handle_; - ThreadPriority prio_; - EventWrapper* event_; - CriticalSectionWrapper* critsect_stop_; - - HANDLE thread_; - unsigned int id_; - char name_[kThreadMaxNameLength]; - bool set_thread_name_; + static DWORD WINAPI StartThread(void* param); + ThreadRunFunction const run_function_; + void* const obj_; + HANDLE event_; // Used to signal stoppage. + // TODO(tommi): Consider having a SetPriority method instead of this variable. + ThreadPriority prio_; + HANDLE thread_; + const std::string name_; + rtc::ThreadChecker main_thread_; }; } // namespace webrtc