Delete SignalThread class.

webrtc/base/nethelpers.cc

Index: webrtc/base/nethelpers.cc
diff --git a/webrtc/base/nethelpers.cc b/webrtc/base/nethelpers.cc
index 6c11ef8c38fa04800df834bc8fa0f0f8939b3b89..2c920e73476b3e462ce6c020711effb7a0c9eb43 100644
--- a/webrtc/base/nethelpers.cc
+++ b/webrtc/base/nethelpers.cc
@@ -26,12 +26,14 @@
 #endif  // defined(WEBRTC_POSIX) && !defined(__native_client__)
 
 #include "webrtc/base/byteorder.h"
+#include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
-#include "webrtc/base/signalthread.h"
+#include "webrtc/base/thread.h"
 
 namespace rtc {
 
+namespace {
 int ResolveHostname(const std::string& hostname, int family,
                     std::vector<IPAddress>* addresses) {
 #ifdef __native_client__
@@ -81,20 +83,27 @@ int ResolveHostname(const std::string& hostname, int family,
   return 0;
 #endif  // !__native_client__
 }
+}  // namespace
 
 // AsyncResolver
 AsyncResolver::AsyncResolver()
-    : SignalThread(false /* use_socket_server */), error_(-1) {}
+    : main_(Thread::Current()),

[tommi, 2017/06/08 08:02:06]

one other idea for a variable name could be |construction_thread_|.

+      worker_(ThreadEntry, this, "AsyncResolver"),

[tommi, 2017/06/08 08:02:05]

nit: worker_(&ThreadEntry, this, "AsyncResolver"),

[nisse-webrtc, 2017/06/08 09:52:33]

Done.

+      error_(-1) {}

[tommi, 2017/06/08 08:02:07]

nit: you could initialize this value inline in the header

[nisse-webrtc, 2017/06/08 09:52:33]

Done.

 
-AsyncResolver::~AsyncResolver() = default;
+AsyncResolver::~AsyncResolver() {
+  RTC_DCHECK(main_->IsCurrent());
+  worker_.Stop();
+}
 
 void AsyncResolver::Start(const SocketAddress& addr) {
+  RTC_DCHECK(main_->IsCurrent());

[tommi, 2017/06/08 08:02:06]

also add:

RTC_DCHECK(!worker_.IsRunning());

   addr_ = addr;
-  // SignalThred Start will kickoff the resolve process.
-  SignalThread::Start();
+  worker_.Start();
 }
 
 bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const {

[tommi, 2017/06/08 08:02:06]

on what thread does this method run?

[nisse-webrtc, 2017/06/08 09:52:33]

Don't know. My understanding is that the application gets the SignalDone on the
construction_thread_, and it might call back to get the address from that
thread. But it should be safe to call these const accessor functions from any
threads during the time between SignalDone and object destruction.

+  rtc::CritScope cs_(&lock_);
   if (error_ != 0 || addresses_.empty())
     return false;
 
@@ -109,22 +118,40 @@ bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const {
 }
 
 int AsyncResolver::GetError() const {
+  rtc::CritScope cs_(&lock_);

[tommi, 2017/06/08 08:02:08]

this sort of pattern always feels strange to me.  grabbing a lock, cache a
return value, release the lock and the caller doesn't have any guarantees that
the returned value is correct.

If this method is called on the same thread as it is set on, then that concern
goes away and we also don't need a lock.  Do we know on what thread this is
called?

[nisse-webrtc, 2017/06/08 09:52:33]

Agreed, this use implies a pretty heavy memory barrier, but no real
synchronization.
As far as I understand, it should be called only after SignalDone, but from any
thread. Similar to GetResolvedAddress.

   return error_;
 }
 
 void AsyncResolver::Destroy(bool wait) {
-  SignalThread::Destroy(wait);
+  RTC_DCHECK(main_->IsCurrent());
+  // If we don't wait here, we will nevertheless wait in the destructor.
+  if (wait) {
+    worker_.Stop();
+  }
 }
 
 void AsyncResolver::DoWork() {

[tommi, 2017/06/08 08:02:06]

would like to have a thread check for this method (all methods in fact)

[nisse-webrtc, 2017/06/08 09:52:33]

It's private and with only a single call site. I'm not used to thread checks
involving platform thread, but you men something like

  RTC_DCHECK(IsThreadRefEqual(worker_.GetThreadRef(), CurrentThreadRef()));

?

-  error_ = ResolveHostname(addr_.hostname().c_str(), addr_.family(),
-                           &addresses_);
+  {
+    rtc::CritScope cs_(&lock_);
+    error_ = ResolveHostname(addr_.hostname().c_str(), addr_.family(),

[tommi, 2017/06/08 08:02:06]

what about not touching addr_ on any other thread and just post a copy of the
result to the construction thread once resolved?

Also, instead of a dedicated thread, a TaskQueue could be used to just do a
PostTaskAndReply in order to resolve the address.  That would also allow
injecting an external queue.

[nisse-webrtc, 2017/06/08 09:52:33]

|addr_| is written in Start (on construction_thread_), read here and in
GetResolvedAddress().

The result is stored in |addresses_|, a vector of IPAddress. We could pass this
and any error back to |construction_thread_| with the invoke. We would then no
longer need to write to any member variable on the worker_ thread. Makes sense,
I'll give it a try.
It kind-of matches the PostTaskAndReply, except that the reply needs to go back
to |construction_thread_| rather than to a task queue. So I'm not sure. If you
can explain how to do it and it becomes simpler than using PlatformThread and an
Invoke back to the construction thread on completion, I'm happy to try it.

+                             &addresses_);
+  }
+  // Ensure SignalDone is called on the main thread.
+  main_->Invoke<void>(RTC_FROM_HERE,

[nisse-webrtc, 2017/06/02 08:41:04]

The sequence of events is slightly different from the old code using
SignalThread. If I've understood it correctly, it used to work like this:

1. Resolver worker thread completes the lookup, sends a non-blocking message to
the main thread, and exits.

2. Main thread receives the message. It then waits for the resolver worker
thread to really terminate (pthread_join or the like).

3. Main threads calls SignalDone.

Current code lets the resolver worker thread do blocking invoke to the main
thread, which calls SignalDone. And the worker thread don't terminate until
after that callback is complete. Not sure if this matters. Calling pthread_join
happens in the destructor, and in addition, if the Destroy method is called with
|wait| true (never happens in webrtc code).

[Taylor Brandstetter, 2017/06/05 22:33:25]

The new sequence seems better actually. If the worker thread is waiting on the
main thread rather than the other way around, the main thread will be freed
again quicker.

+                      rtc::Bind(&AsyncResolver::OnWorkDone, this));
 }
 
 void AsyncResolver::OnWorkDone() {

[Taylor Brandstetter, 2017/06/05 22:33:25]

nit: Rename to "SignalWorkDone"?

[nisse-webrtc, 2017/06/07 07:17:31]

Done.

+  RTC_DCHECK(main_->IsCurrent());
   SignalDone(this);
 }
 
+// static
+void AsyncResolver::ThreadEntry(void *p) {
+  AsyncResolver* resolver = static_cast<AsyncResolver*>(p);
+  resolver->DoWork();
+}
+
 const char* inet_ntop(int af, const void *src, char* dst, socklen_t size) {
 #if defined(WEBRTC_WIN)
   return win32_inet_ntop(af, src, dst, size);