From 05255b0e8a4216a14e34b49ef8cbacb7defd8f3c Mon Sep 17 00:00:00 2001 From: perkj Date: Wed, 6 Apr 2016 01:28:30 -0700 Subject: [PATCH] Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1 id:1 of https://codereview.webrtc.org/1859933002/ ) Reason for revert: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469/steps/peerconnection_unittests/logs/stdio e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 Original issue's description: > Changed P2PTestConductor to use a separate WorkerThread. > > P2PTestConductor currently use the current thread both as a signaling thread and a worker thread. Although convenient while debugging, it can also hide real bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the worker thread is deadlocked in the track proxy due to that the worker thread waits for the signaling thread but the proxy in turns invokes the worker thread..... That bug was only discovered on Android. I suggest we let the P2PTestConductor use a separate thread as a worker thread to better cover how PeerConnections are used in reality. > > BUG=webrtc:5426 > > Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4 > Cr-Commit-Position: refs/heads/master@{#12252} TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5426 Review URL: https://codereview.webrtc.org/1866503003 Cr-Commit-Position: refs/heads/master@{#12255} --- webrtc/api/peerconnection_unittest.cc | 53 +++++++++++---------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/webrtc/api/peerconnection_unittest.cc b/webrtc/api/peerconnection_unittest.cc index 99be22f459..67b4efbc73 100644 --- a/webrtc/api/peerconnection_unittest.cc +++ b/webrtc/api/peerconnection_unittest.cc @@ -155,11 +155,10 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, const MediaConstraintsInterface* constraints, const PeerConnectionFactory::Options* options, rtc::scoped_ptr dtls_identity_store, - bool prefer_constraint_apis, - rtc::Thread* worker_thread) { + bool prefer_constraint_apis) { PeerConnectionTestClient* client(new PeerConnectionTestClient(id)); if (!client->Init(constraints, options, std::move(dtls_identity_store), - prefer_constraint_apis, worker_thread)) { + prefer_constraint_apis)) { delete client; return nullptr; } @@ -169,28 +168,24 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, static PeerConnectionTestClient* CreateClient( const std::string& id, const MediaConstraintsInterface* constraints, - const PeerConnectionFactory::Options* options, - rtc::Thread* worker_thread) { + const PeerConnectionFactory::Options* options) { rtc::scoped_ptr dtls_identity_store( rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore() : nullptr); - return CreateClientWithDtlsIdentityStore(id, constraints, options, - std::move(dtls_identity_store), - true, worker_thread); + return CreateClientWithDtlsIdentityStore( + id, constraints, options, std::move(dtls_identity_store), true); } static PeerConnectionTestClient* CreateClientPreferNoConstraints( const std::string& id, - const PeerConnectionFactory::Options* options, - rtc::Thread* worker_thread) { + const PeerConnectionFactory::Options* options) { rtc::scoped_ptr dtls_identity_store( rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore() : nullptr); - return CreateClientWithDtlsIdentityStore(id, nullptr, options, - std::move(dtls_identity_store), - false, worker_thread); + return CreateClientWithDtlsIdentityStore( + id, nullptr, options, std::move(dtls_identity_store), false); } ~PeerConnectionTestClient() { @@ -805,8 +800,7 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, const MediaConstraintsInterface* constraints, const PeerConnectionFactory::Options* options, rtc::scoped_ptr dtls_identity_store, - bool prefer_constraint_apis, - rtc::Thread* worker_thread) { + bool prefer_constraint_apis) { EXPECT_TRUE(!peer_connection_); EXPECT_TRUE(!peer_connection_factory_); if (!prefer_constraint_apis) { @@ -815,7 +809,7 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, prefer_constraint_apis_ = prefer_constraint_apis; rtc::scoped_ptr port_allocator( - new cricket::FakePortAllocator(worker_thread, nullptr)); + new cricket::FakePortAllocator(rtc::Thread::Current(), nullptr)); fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); if (fake_audio_capture_module_ == nullptr) { @@ -824,8 +818,9 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, fake_video_decoder_factory_ = new FakeWebRtcVideoDecoderFactory(); fake_video_encoder_factory_ = new FakeWebRtcVideoEncoderFactory(); peer_connection_factory_ = webrtc::CreatePeerConnectionFactory( - worker_thread, rtc::Thread::Current(), fake_audio_capture_module_, - fake_video_encoder_factory_, fake_video_decoder_factory_); + rtc::Thread::Current(), rtc::Thread::Current(), + fake_audio_capture_module_, fake_video_encoder_factory_, + fake_video_decoder_factory_); if (!peer_connection_factory_) { return false; } @@ -1024,9 +1019,7 @@ class P2PTestConductor : public testing::Test { P2PTestConductor() : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()) { - RTC_CHECK(worker_thread_.Start()); - } + ss_scope_(ss_.get()) {} bool SessionActive() { return initiating_client_->SessionActive() && @@ -1134,11 +1127,11 @@ class P2PTestConductor : public testing::Test { bool CreateTestClientsThatPreferNoConstraints() { initiating_client_.reset( - PeerConnectionTestClient::CreateClientPreferNoConstraints( - "Caller: ", nullptr, &worker_thread_)); + PeerConnectionTestClient::CreateClientPreferNoConstraints("Caller: ", + nullptr)); receiving_client_.reset( - PeerConnectionTestClient::CreateClientPreferNoConstraints( - "Callee: ", nullptr, &worker_thread_)); + PeerConnectionTestClient::CreateClientPreferNoConstraints("Callee: ", + nullptr)); if (!initiating_client_ || !receiving_client_) { return false; } @@ -1158,9 +1151,9 @@ class P2PTestConductor : public testing::Test { MediaConstraintsInterface* recv_constraints, PeerConnectionFactory::Options* recv_options) { initiating_client_.reset(PeerConnectionTestClient::CreateClient( - "Caller: ", init_constraints, init_options, &worker_thread_)); + "Caller: ", init_constraints, init_options)); receiving_client_.reset(PeerConnectionTestClient::CreateClient( - "Callee: ", recv_constraints, recv_options, &worker_thread_)); + "Callee: ", recv_constraints, recv_options)); if (!initiating_client_ || !receiving_client_) { return false; } @@ -1261,8 +1254,7 @@ class P2PTestConductor : public testing::Test { // Make sure the new client is using a different certificate. return PeerConnectionTestClient::CreateClientWithDtlsIdentityStore( "New Peer: ", &setup_constraints, nullptr, - std::move(dtls_identity_store), prefer_constraint_apis_, - &worker_thread_); + std::move(dtls_identity_store), prefer_constraint_apis_); } void SendRtpData(webrtc::DataChannelInterface* dc, const std::string& data) { @@ -1302,9 +1294,6 @@ class P2PTestConductor : public testing::Test { } private: - // |worker_thread_| is used by both |initiating_client_| and - // |receiving_client_|. Must be destroyed last. - rtc::Thread worker_thread_; rtc::scoped_ptr pss_; rtc::scoped_ptr ss_; rtc::SocketServerScope ss_scope_;