Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915253002:
Delete SignalThread class. (Closed)
|
Created:
3 years, 6 months ago by nisse-webrtc Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, kwiberg-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete SignalThread class.
Rewrite AsyncResolver to use PlatformThread directly, not
SignalThread, and update includes of peerconnection client to not
depend on signalthread.h.
BUG=webrtc:6424, webrtc:7723
Review-Url: https://codereview.webrtc.org/2915253002
Cr-Commit-Position: refs/heads/master@{#18833}
Committed: https://chromium.googlesource.com/external/webrtc/+/bc8feda1db02b2a9b501e4aa43926ca7e861b638
Patch Set 1 #Patch Set 2 : Remove thread check on AsyncResolver::GetResolvedAddress and AsyncResolver::GetError. #Patch Set 3 : Invoke SignalDone on the main thread. #
Total comments: 31
Patch Set 4 : Address Taylor's comments, and delete lock. #Patch Set 5 : Address nits. #Patch Set 6 : Write the member variables for the result only on the |construction_thread_|. #
Total comments: 16
Patch Set 7 : Address comments on const and std::move. #Patch Set 8 : Fix resolver memory leak. #Patch Set 9 : Use a task queue. #
Total comments: 5
Patch Set 10 : Address nits. #Patch Set 11 : Implement Destroy logic, using new method MessageQueue::PostFunctor. #
Total comments: 12
Patch Set 12 : Fix state update. #Patch Set 13 : Address Karl's comments. #Patch Set 14 : Fix template magic. Naming and formatting fixes. #
Total comments: 12
Patch Set 15 : Fix issue with post on destruction, adding one level of indirection. #Patch Set 16 : Rename Call --> Run and Callable --> Runnable. #Patch Set 17 : Fix for the case of SignalDone calling Destroy. #Patch Set 18 : Rebase, a bit tricky due to move base --> rtc_base. #Patch Set 19 : Added TODO comment on posting of QueuedTask. #
Total comments: 2
Patch Set 20 : Another TODO on moving to QueuedTask. #Messages
Total messages: 45 (14 generated)
nisse@webrtc.org changed reviewers: + deadbeef@webrtc.org, tommi@webrtc.org
PTAL, and see my own code comments. I'm not really familiar with PlatformThread, or what the threading conventions really are for the AsyncResolverInterface. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:140: main_->Invoke<void>(RTC_FROM_HERE, 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). https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:54: rtc::CriticalSection lock_; The use of this lock is maybe overly conservative. I don't know if the sigslot machinery itself implies any memory barriers, but I think the Invoke from the worker thread back to the main thread should be enough to make stores from the worker thread visible to other threads, right?
Description was changed from ========== Delete SignalThread class. Rewrite AsyncResolver to use PlatformThread directly, not SignalThread, and update includes of peerconnection client to not depend on signalthread.h. BUG=webrtc:6424 ========== to ========== Delete SignalThread class. Rewrite AsyncResolver to use PlatformThread directly, not SignalThread, and update includes of peerconnection client to not depend on signalthread.h. BUG=webrtc:6424,webrtc:7723 ==========
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:140: main_->Invoke<void>(RTC_FROM_HERE, On 2017/06/02 08:41:04, nisse-webrtc wrote: > 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). 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. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:144: void AsyncResolver::OnWorkDone() { nit: Rename to "SignalWorkDone"? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:33: // the SignalDone from AsyncResolverInterface when the operation completes. nit: Could you add a comment saying that SignalDone will be fired from the same thread on which the AsyncResolver was constructed? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:48: // Thread main function nit: Period? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:54: rtc::CriticalSection lock_; On 2017/06/02 08:41:04, nisse-webrtc wrote: > The use of this lock is maybe overly conservative. > > I don't know if the sigslot machinery itself implies any memory barriers, but I > think the Invoke from the worker thread back to the main thread should be enough > to make stores from the worker thread visible to other threads, right? It should be enough. Can you try removing the lock? If there are TSan warnings I'd be surprised.
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:144: void AsyncResolver::OnWorkDone() { On 2017/06/05 22:33:25, Taylor Brandstetter wrote: > nit: Rename to "SignalWorkDone"? Done. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:33: // the SignalDone from AsyncResolverInterface when the operation completes. On 2017/06/05 22:33:25, Taylor Brandstetter wrote: > nit: Could you add a comment saying that SignalDone will be fired from the same > thread on which the AsyncResolver was constructed? Done. But maybe that comment belongs with the AsyncResolverInterface class (which doesn't mention threads at all)? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:54: rtc::CriticalSection lock_; On 2017/06/05 22:33:25, Taylor Brandstetter wrote: > On 2017/06/02 08:41:04, nisse-webrtc wrote: > > The use of this lock is maybe overly conservative. > > > > I don't know if the sigslot machinery itself implies any memory barriers, but > I > > think the Invoke from the worker thread back to the main thread should be > enough > > to make stores from the worker thread visible to other threads, right? > > It should be enough. Can you try removing the lock? If there are TSan warnings > I'd be surprised. Done.
Ping? Tommi, I think you said you wanted to provide some comments?
sorry, I thought I had submitted the below comments. better late than never :-/ Let me know what you think about the TQ idea https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:90: : main_(Thread::Current()), one other idea for a variable name could be |construction_thread_|. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:91: worker_(ThreadEntry, this, "AsyncResolver"), nit: worker_(&ThreadEntry, this, "AsyncResolver"), https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:92: error_(-1) {} nit: you could initialize this value inline in the header https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:100: RTC_DCHECK(main_->IsCurrent()); also add: RTC_DCHECK(!worker_.IsRunning()); https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:105: bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const { on what thread does this method run? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:113: addr->SetResolvedIP(addresses_[i]); calling out while holding a lock, would be nice to avoid doing that. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:121: rtc::CritScope cs_(&lock_); 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? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:133: void AsyncResolver::DoWork() { would like to have a thread check for this method (all methods in fact) https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:136: error_ = ResolveHostname(addr_.hostname().c_str(), addr_.family(), 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. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:49: static void ThreadEntry(void *p); void* p (or even |param|) can you run git cl format? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:50: void DoWork(); can we have slightly more descriptive names for these methods? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:53: rtc::Thread* main_; nit: I'd prefer |thread_| to |main_|. Actually... what thread is this? If there's already a general name for it (e.g. the 'signaling' or 'worker' threads from libjingle), we could name it accordingly.
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:91: worker_(ThreadEntry, this, "AsyncResolver"), On 2017/06/08 08:02:05, tommi (wëbrtc) wrote: > nit: worker_(&ThreadEntry, this, "AsyncResolver"), Done. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:92: error_(-1) {} On 2017/06/08 08:02:07, tommi (wëbrtc) wrote: > nit: you could initialize this value inline in the header Done. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:105: bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const { On 2017/06/08 08:02:06, tommi (wëbrtc) wrote: > on what thread does this method run? 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. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:113: addr->SetResolvedIP(addresses_[i]); On 2017/06/08 08:02:09, tommi (wëbrtc) wrote: > calling out while holding a lock, would be nice to avoid doing that. SetResolvedAddress is a cheap setter, as far as I understand, not a callback to the application. Anyway, the lock was deleted in the next patch set. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:121: rtc::CritScope cs_(&lock_); On 2017/06/08 08:02:08, tommi (wëbrtc) wrote: > 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. Agreed, this use implies a pretty heavy memory barrier, but no real synchronization. > 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? As far as I understand, it should be called only after SignalDone, but from any thread. Similar to GetResolvedAddress. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:133: void AsyncResolver::DoWork() { On 2017/06/08 08:02:06, tommi (wëbrtc) wrote: > would like to have a thread check for this method (all methods in fact) 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())); ? https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc... webrtc/base/nethelpers.cc:136: error_ = ResolveHostname(addr_.hostname().c_str(), addr_.family(), On 2017/06/08 08:02:06, tommi (wëbrtc) wrote: > what about not touching addr_ on any other thread and just post a copy of the > result to the construction thread once resolved? |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. > 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. 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. https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:49: static void ThreadEntry(void *p); On 2017/06/08 08:02:10, tommi (wëbrtc) wrote: > void* p > (or even |param|) > > can you run git cl format? Done.
lgtm https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#... webrtc/base/nethelpers.h:33: // the SignalDone from AsyncResolverInterface when the operation completes. On 2017/06/07 07:17:31, nisse-webrtc wrote: > On 2017/06/05 22:33:25, Taylor Brandstetter wrote: > > nit: Could you add a comment saying that SignalDone will be fired from the > same > > thread on which the AsyncResolver was constructed? > > Done. But maybe that comment belongs with the AsyncResolverInterface class > (which doesn't mention threads at all)? The comment applies to this class's constructor specifically, so this seems like the right place.
Tommi, any comments on the updated cl? https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:137: construction_thread_->Invoke<void>( Actually, I don't think there's any need for this method to block waiting for call to complete on the |construction_thread_|. Is there any easy way to do a "non-blocking invoke", making copies of the arguments as needed?
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:137: construction_thread_->Invoke<void>( On 2017/06/09 09:44:52, nisse-webrtc wrote: > Actually, I don't think there's any need for this method to block waiting for > call to complete on the |construction_thread_|. Is there any easy way to do a > "non-blocking invoke", making copies of the arguments as needed? Using an AsyncInvoker is the most convenient way I know of.
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:102: worker_.Start(); implementing a custom QueuedTask that handles the PostTaskAndReply logic (posting to construction_thread_ once done) could be a way to use a taskqueue + PostTask. https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:126: if (wait) { why is wait == false supported? https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:139: error, addresses)); std::move? (or will address be implicitly moved?) https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:145: addresses_ = addresses; std::move https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:150: void AsyncResolver::ThreadEntry_w(void* p) { What about using a TaskQueue? https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:54: void ResolveDone(int error, std::vector<IPAddress> addresses); passing by value intentionally? If so, a comment about ownership passing would be nice https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:56: rtc::Thread* construction_thread_; rtc::Thread* const construction_thread_; https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:57: PlatformThread worker_; nit: resolve_thread_
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:102: worker_.Start(); On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > implementing a custom QueuedTask that handles the PostTaskAndReply logic > (posting to construction_thread_ once done) could be a way to use a taskqueue + > PostTask. I'll give it a try. https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:126: if (wait) { On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > why is wait == false supported? It's part of the AsyncResolverInterface, which I don't intend to change in this cl. And it's always false, when called from within webrtc, see, e.g., https://cs.chromium.org/chromium/src/third_party/webrtc/base/physicalsocketse... For a task queue implementation, should wait == true imply destruction of the task queue? If the task queue is destroyed while some task in the queue is running, I imagine the destruction will block until the current task is complete? And in the case of a task queue with a dedicated thread, imply a pthread_join? https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:139: error, addresses)); On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > std::move? (or will address be implicitly moved?) Done. https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:54: void ResolveDone(int error, std::vector<IPAddress> addresses); On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > passing by value intentionally? If so, a comment about ownership passing would > be nice I was aiming to keep this as simple as possible, and I don't think performance matters much here. Do you think pass-by-value + std::move is good enough? I you prefer, I could change it to std::unique_ptr<std::vector>. https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:56: rtc::Thread* construction_thread_; On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > rtc::Thread* const construction_thread_; Done. https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.h... webrtc/base/nethelpers.h:57: PlatformThread worker_; On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > nit: resolve_thread_ Not doing this now, since I'll attempt to replace it with a TaskQueue.
On 2017/06/15 11:33:48, nisse-webrtc wrote: > https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc > File webrtc/base/nethelpers.cc (right): > > https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.c... > webrtc/base/nethelpers.cc:102: worker_.Start(); > On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > > implementing a custom QueuedTask that handles the PostTaskAndReply logic > > (posting to construction_thread_ once done) could be a way to use a taskqueue > + > > PostTask. > > I'll give it a try. Done in ps#9. ps#8 fixes a memory leak (unclear why it wasn't detected earlier) which made the test TurnPortTest.TestResolverShutdown fail, detecting fd leaks related to the leaked resolver and libevent state.
lgtm https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:92: : construction_thread_(Thread::Current()) {} add RTC_DCHECK that construction_thread_ is never null in the ctor (catch it early) https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:203: resolver_(), nit: remove from the initializer list since it'll be default constructed https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:238: resolver_(), nit: remove (we don't include default constructed stl in initializer lists)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:92: : construction_thread_(Thread::Current()) {} On 2017/06/19 13:01:47, tommi (wëbrtc) wrote: > add RTC_DCHECK that construction_thread_ is never null in the ctor (catch it > early) Done. https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:203: resolver_(), On 2017/06/19 13:01:47, tommi (wëbrtc) wrote: > nit: remove from the initializer list since it'll be default constructed Done. Here and below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/25688)
On 2017/06/19 14:07:21, nisse-webrtc wrote: > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc > File webrtc/base/nethelpers.cc (right): > > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.c... > webrtc/base/nethelpers.cc:92: : construction_thread_(Thread::Current()) {} > On 2017/06/19 13:01:47, tommi (wëbrtc) wrote: > > add RTC_DCHECK that construction_thread_ is never null in the ctor (catch it > > early) > > Done. > > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:203: resolver_(), > On 2017/06/19 13:01:47, tommi (wëbrtc) wrote: > > nit: remove from the initializer list since it'll be default constructed > > Done. Here and below. Hmm, it turns out that I've misunderstood the Destroy method. This is my current understanding: When the application calls AsyncResolverInterface::Destroy, that means that the application promises it won't call any further methods, and if a lookup is pending, it's no longer interested in any result. If wait is true, Destroy should block until the worker thread is finished, and then delete the resolver. But if wait is false, we need to check if any lookup is pending. If it is, we'll delete the object right away. But if not, the worker thread essentially takes ownership of the object, and it should be deleted when the worker thread is done. I'll try to implement that, and undo the change in ps#8. I think we can do it without locking, since ResolveDone and Destroy are called on the same thread. However, the blocking invoke is going to be a problem. For it to work properly, the worker thread needs to send a message and then exit without blocking.
On 2017/06/19 15:17:47, nisse-webrtc wrote: > I'll try to implement that, and undo the change in ps#8. I think we can do it > without locking, since ResolveDone and Destroy are called on the same thread. > However, the blocking invoke is going to be a problem. For it to work properly, > the worker thread needs to send a message and then exit without blocking. I've given it a try, see ps#11. Some notes: The cleanup in MesageQueue::Clear (called by the Thread destructor) deletes the MessageData corresponding to discarded messages, but not the MessageHandler. So to make transfer of ownership of the functor work, we have to attach it to the MessageData, not the MessageHandler. And use a global stateless MessageHandler. I'd like to name the method just MessageQueue::Post, but to make that work, we need some magic to make the template require the second argument to be a functor, otherwise we get compilation errors when the template is applied to Post calls with two arguments and the second argument is a MessageHandler.
nisse@webrtc.org changed reviewers: + kwiberg@webrtc.org
Karl, can you have a look at the new MessageQueue::PostFunctor method?
The MessageQueue::PostFunctor looks good, but I have some comments. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:57: } Define this function inline? There's no downside to that when you define a class in a .cc file. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:548: RTC_DEFINE_STATIC_LOCAL(FunctorPostMessageHandler, handler, ()); Why not just FunctorPostMessageHandler handler; ? That class has a trivial default constructor and a trivial destructor, since it has no members. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void call(void) = 0; "Call" https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:257: // above Post method with optional arguments omitted. Should be simple to do by e.g. using enable_if to assert that FunctorT isn't a pointer type (with std::is_pointer).
I've addressed Karl's comment. I see one remaining problem: The message posted to the construction_thread_ may outlive the target object. I see two ways to solve this: 1. Somehow expose the target object in th message, and generalize the MessageQueue::Clear method so we can remove messages targeting a specific object, and call that from the AsyncResolver destructor, after destroying the task_queue_ (which implies pthread_join()). Should be perfectly thread safe. 2. And one level of indirection, via a refcounted object with a pointer to the AsyncResolver, and post an invocation of a method on this trampoline object, rather than on the AsyncResolver itself. This pointer can be cleared in the AsyncResolver destructor, and checked in the posted code block, and this should also be safe since they'll be running on the same thread. I think I'll go with option (2) for now, but I think option (1) would be generally useful, and would be easier to get right than the related logic in AsyncInvoker. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:57: } On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > Define this function inline? There's no downside to that when you define a class > in a .cc file. Done. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:548: RTC_DEFINE_STATIC_LOCAL(FunctorPostMessageHandler, handler, ()); On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > Why not just > > FunctorPostMessageHandler handler; > > ? That class has a trivial default constructor and a trivial destructor, since > it has no members. It has to outlive this scope, since it's accessed (vtable lookup for OnMessage) by the time the target thread gets to process the posted message. But if you say a plain "static" definition works fine, I'll do that. The vtable is initialized properly at link time, then? https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void call(void) = 0; On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > "Call" Done. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:257: // above Post method with optional arguments omitted. On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > Should be simple to do by e.g. using enable_if to assert that FunctorT isn't a > pointer type (with std::is_pointer). Done. https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:381: return; Unrelated to this cl, but shouldn't this code call delete pdata before returning, analogous to messages removed by Clear()? It's also unclear to me what synchronization is intended with the use of the |stop_| flag. What if Post method could gets to this check, sees IsQuitting() false, and then is preempted? And by the time it's rescheduled, after lots of other action, IsQuitting() might be true. When can that happen? And if it's harmless, why do we have the check in the first place?
lgtm for the new MessageQueue::PostFunctor method https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:548: RTC_DEFINE_STATIC_LOCAL(FunctorPostMessageHandler, handler, ()); On 2017/06/27 08:27:17, nisse-webrtc wrote: > On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > > Why not just > > > > FunctorPostMessageHandler handler; > > > > ? That class has a trivial default constructor and a trivial destructor, since > > it has no members. > > It has to outlive this scope, since it's accessed (vtable lookup for OnMessage) > by the time the target thread gets to process the posted message. > > But if you say a plain "static" definition works fine, I'll do that. The vtable > is initialized properly at link time, then? Ah, right, it has a vtable. But yes, as of C++11, function-local static variables are threadsafely initialized (with a mutex if necessary, but as you point out vtables might be initialized at link time), and vtables shouldn't have to cause any work in the destructor, so a plain "static" definition works fine. (RTC_DEFINE_STATIC_LOCAL relies on the same mechanisms for synchronizing the creation of the object (but because of the heap allocation, we know there's runtime initialization that needs to be mutexed). The only difference is that it leaks the object into the heap, so that its destructor is never run, but that should make no difference in this case.)
got one crazy-ish question/proposal that I'd like to hear what you think about (karl as well) https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:546: { can you run git cl format? https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:381: return; On 2017/06/27 08:27:17, nisse-webrtc wrote: > Unrelated to this cl, but shouldn't this code call > > delete pdata > > before returning, analogous to messages removed by Clear()? > > It's also unclear to me what synchronization is intended with the use of the > |stop_| flag. What if Post method could gets to this check, sees IsQuitting() > false, and then is preempted? And by the time it's rescheduled, after lots of > other action, IsQuitting() might be true. When can that happen? And if it's > harmless, why do we have the check in the first place? depends - I think there might be call sites that control deletion of pdata. It's too bad that there isn't a return value or that ownership of pdata isn't passed via unique_ptr. https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; virtual void Call() = 0; Btw, this seems to now basically seem to be the same as QueuedTask(), so would it make sense to encourage use of TaskQueue instead (in a comment)? Possibly even call the method Run()? hmm...or maybe that would just make things confusing. Another thing to consider is if we should just have an approach like this instead :) class QueuedTaskAdapter : public MessageData { public: < ctor + dtor + additional fluff> std::unique_ptr<QueuedTask> task_; }; One benefit of that would be that instead of adding one more interface to MessageData et al, we introduce an explicit adapter between TaskQueue and libjingle queues, that encourage use of the TaskQueue type. https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:159: void Call(void) override { functor_(); } same here (no 'void' when there aren't any arguments) https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:146: // we will never handle. this comment doesn't really explain why it's OK to sometimes delete |this| and sometimes not. Especially considering that we might do that depending on internal state, I'm wondering how the external code can know what to do with the pointer to the AsyncResolver object
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > virtual void Call() = 0; > > Btw, this seems to now basically seem to be the same as QueuedTask(), so would > it make sense to encourage use of TaskQueue instead (in a comment)? Possibly > even call the method Run()? hmm...or maybe that would just make things > confusing. > > Another thing to consider is if we should just have an approach like this > instead :) > > class QueuedTaskAdapter : public MessageData { > public: > < ctor + dtor + additional fluff> > std::unique_ptr<QueuedTask> task_; > }; > > One benefit of that would be that instead of adding one more interface to > MessageData et al, we introduce an explicit adapter between TaskQueue and > libjingle queues, that encourage use of the TaskQueue type. sgtm---the new Post() call could take a unique_ptr<QueuedTask> argument instead of a FunctorT, and everything else could be hidden in the implementation. I like QueuedTask, except for the thing with the return value from Run() that says whether the queue should delete the QueuedTask or relinquish ownership.
https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:546: { On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > can you run git cl format? Already done, in ps#14. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:548: RTC_DEFINE_STATIC_LOCAL(FunctorPostMessageHandler, handler, ()); On 2017/06/27 09:16:28, kwiberg-webrtc wrote: > On 2017/06/27 08:27:17, nisse-webrtc wrote: > > On 2017/06/26 13:07:48, kwiberg-webrtc wrote: > > > Why not just > > > > > > FunctorPostMessageHandler handler; > > > > > > ? That class has a trivial default constructor and a trivial destructor, > since > > > it has no members. > > > > It has to outlive this scope, since it's accessed (vtable lookup for > OnMessage) > > by the time the target thread gets to process the posted message. > > > > But if you say a plain "static" definition works fine, I'll do that. The > vtable > > is initialized properly at link time, then? > > Ah, right, it has a vtable. But yes, as of C++11, function-local static > variables are threadsafely initialized (with a mutex if necessary, but as you > point out vtables might be initialized at link time), and vtables shouldn't have > to cause any work in the destructor, so a plain "static" definition works fine. > (RTC_DEFINE_STATIC_LOCAL relies on the same mechanisms for synchronizing the > creation of the object (but because of the heap allocation, we know there's > runtime initialization that needs to be mutexed). The only difference is that it > leaks the object into the heap, so that its destructor is never run, but that > should make no difference in this case.) Good. But it still has to be in a local scope? (If we implement some cancel mechanism as I outlined as option (1), it would help to move it into a global unnamed namespace, but I'm not sure if that would involved constructor magic before main(). https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.cc:381: return; On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > On 2017/06/27 08:27:17, nisse-webrtc wrote: > > Unrelated to this cl, but shouldn't this code call > > > > delete pdata > > > > before returning, analogous to messages removed by Clear()? > > > > It's also unclear to me what synchronization is intended with the use of the > > |stop_| flag. What if Post method could gets to this check, sees IsQuitting() > > false, and then is preempted? And by the time it's rescheduled, after lots of > > other action, IsQuitting() might be true. When can that happen? And if it's > > harmless, why do we have the check in the first place? > > depends - I think there might be call sites that control deletion of pdata. It's > too bad that there isn't a return value or that ownership of pdata isn't passed > via unique_ptr. I was thinking that we should handle discarded messages in the same way, both if they're discarded based on the quit flag here, or if they're put onto the list but discarded later by the Clear() method. It's hard for caller to observe any difference. The comment just above the MessageData class says "App manages lifetime, except when messages are purged", which isn't crystal clear. https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > virtual void Call() = 0; Done. > Btw, this seems to now basically seem to be the same as QueuedTask(), so would > it make sense to encourage use of TaskQueue instead (in a comment)? Possibly > even call the method Run()? hmm...or maybe that would just make things > confusing. I've thought about adding a method MessageQueue::PostTask(std::unique_ptr<QueuedTask> task); to make it possible to use an rtc::Thread more or less the same way as a task queue. Which seems to be pretty close to what you are suggesting. Would that make sense? And renaming Call() --> Run() would improve consistency, I think. Do we need to rename the CallableData interface too, and how? https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:146: // we will never handle. On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > this comment doesn't really explain why it's OK to sometimes delete |this| and > sometimes not. Especially considering that we might do that depending on > internal state, I'm wondering how the external code can know what to do with the > pointer to the AsyncResolver object The AsyncResolverInterface is poorly documented, but the way I understand it, after Destroy() returns, the user should consider the object deleted and not access it in any way. Maybe users of the class also shouldn't call the destructor explicitly, perhaps ~AsyncResolverInterface() should be demoted from public to protected. But in any case, it's invalid to explicitly destruct the object after calling the Destroy method.
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/28 11:47:54, nisse-webrtc wrote: > On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > > virtual void Call() = 0; > > Done. > > > Btw, this seems to now basically seem to be the same as QueuedTask(), so would > > it make sense to encourage use of TaskQueue instead (in a comment)? Possibly > > even call the method Run()? hmm...or maybe that would just make things > > confusing. > > I've thought about adding a method > > MessageQueue::PostTask(std::unique_ptr<QueuedTask> task); > > to make it possible to use an rtc::Thread more or less the same way as a task > queue. Which seems to be pretty close to what you are suggesting. Would that > make sense? yes, I think so > And renaming Call() --> Run() would improve consistency, I think. Do we need to > rename the CallableData interface too, and how? Do we need CallableData as an interface in that case? I'm wondering if we could get away with a custom implementation of MessageData that owns a QueuedTask and our handler recognizes. The custom type wouldn't implement a new interface but basically just be a vehicle for carrying the QueuedTask object. The handler then takes care of the logic.
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/28 11:47:54, nisse-webrtc wrote: > On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > > virtual void Call() = 0; > > Done. > > > Btw, this seems to now basically seem to be the same as QueuedTask(), so would > > it make sense to encourage use of TaskQueue instead (in a comment)? Possibly > > even call the method Run()? hmm...or maybe that would just make things > > confusing. > > I've thought about adding a method > > MessageQueue::PostTask(std::unique_ptr<QueuedTask> task); > > to make it possible to use an rtc::Thread more or less the same way as a task > queue. Which seems to be pretty close to what you are suggesting. Would that > make sense? > > And renaming Call() --> Run() would improve consistency, I think. Do we need to > rename the CallableData interface too, and how? I've renamed Call to Run, and Callable to Runnable. https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue... webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/28 13:17:33, tommi (wëbrtc) wrote: > Do we need CallableData as an interface in that case? I'm wondering if we could > get away with a custom implementation of MessageData that owns a QueuedTask and > our handler recognizes. The custom type wouldn't implement a new interface but > basically just be a vehicle for carrying the QueuedTask object. The handler > then takes care of the logic. Sounds like it could work, I'll give it a try, but might not get ready before I leave for vacation. I guess there's no hurry, but if you think the current version of the cl is good enough, we could land it now, and get back to QueuedTask later.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, tommi@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2915253002/#ps360001 (title: "Added TODO comment on posting of QueuedTask.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm as is then with TODOs to do the remaining work https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messageq... File webrtc/rtc_base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messageq... webrtc/rtc_base/messagequeue.h:150: class RunnableData : public MessageData { can you add a TODO here to move over to using QueuedTask instead?
The CQ bit was unchecked by nisse@webrtc.org
https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messageq... File webrtc/rtc_base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messageq... webrtc/rtc_base/messagequeue.h:150: class RunnableData : public MessageData { On 2017/06/29 12:27:52, tommi (wëbrtc) wrote: > can you add a TODO here to move over to using QueuedTask instead? Done.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, deadbeef@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2915253002/#ps380001 (title: "Another TODO on moving to QueuedTask.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1498739935862810,
"parent_rev": "428c9e218538278e6b0db42d1b734431bb432e1a", "commit_rev":
"bc8feda1db02b2a9b501e4aa43926ca7e861b638"}
Message was sent while issue was closed.
Description was changed from ========== Delete SignalThread class. Rewrite AsyncResolver to use PlatformThread directly, not SignalThread, and update includes of peerconnection client to not depend on signalthread.h. BUG=webrtc:6424,webrtc:7723 ========== to ========== Delete SignalThread class. Rewrite AsyncResolver to use PlatformThread directly, not SignalThread, and update includes of peerconnection client to not depend on signalthread.h. BUG=webrtc:6424,webrtc:7723 Review-Url: https://codereview.webrtc.org/2915253002 Cr-Commit-Position: refs/heads/master@{#18833} Committed: https://chromium.googlesource.com/external/webrtc/+/bc8feda1db02b2a9b501e4aa4... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/external/webrtc/+/bc8feda1db02b2a9b501e4aa4...
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.webrtc.org/2979733002/ by deadbeef@webrtc.org. The reason for reverting is: Seems to be causing new crashes, possibly because of changes to the "Destroy(false)" behavior. Will re-land after investigating these crashes more and addressing the root cause.. |
