From 6172401972c54813698d73580779d675d99178b4 Mon Sep 17 00:00:00 2001 From: perkj Date: Wed, 6 Apr 2016 00:02:59 -0700 Subject: [PATCH] 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 Review URL: https://codereview.webrtc.org/1859933002 Cr-Commit-Position: refs/heads/master@{#12252} --- webrtc/api/peerconnection_unittest.cc | 53 ++++++++++++++++----------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/webrtc/api/peerconnection_unittest.cc b/webrtc/api/peerconnection_unittest.cc index 67b4efbc73..99be22f459 100644 --- a/webrtc/api/peerconnection_unittest.cc +++ b/webrtc/api/peerconnection_unittest.cc @@ -155,10 +155,11 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, const MediaConstraintsInterface* constraints, const PeerConnectionFactory::Options* options, rtc::scoped_ptr dtls_identity_store, - bool prefer_constraint_apis) { + bool prefer_constraint_apis, + rtc::Thread* worker_thread) { PeerConnectionTestClient* client(new PeerConnectionTestClient(id)); if (!client->Init(constraints, options, std::move(dtls_identity_store), - prefer_constraint_apis)) { + prefer_constraint_apis, worker_thread)) { delete client; return nullptr; } @@ -168,24 +169,28 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, static PeerConnectionTestClient* CreateClient( const std::string& id, const MediaConstraintsInterface* constraints, - const PeerConnectionFactory::Options* options) { + const PeerConnectionFactory::Options* options, + rtc::Thread* worker_thread) { rtc::scoped_ptr dtls_identity_store( rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore() : nullptr); - return CreateClientWithDtlsIdentityStore( - id, constraints, options, std::move(dtls_identity_store), true); + return CreateClientWithDtlsIdentityStore(id, constraints, options, + std::move(dtls_identity_store), + true, worker_thread); } static PeerConnectionTestClient* CreateClientPreferNoConstraints( const std::string& id, - const PeerConnectionFactory::Options* options) { + const PeerConnectionFactory::Options* options, + rtc::Thread* worker_thread) { rtc::scoped_ptr dtls_identity_store( rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore() : nullptr); - return CreateClientWithDtlsIdentityStore( - id, nullptr, options, std::move(dtls_identity_store), false); + return CreateClientWithDtlsIdentityStore(id, nullptr, options, + std::move(dtls_identity_store), + false, worker_thread); } ~PeerConnectionTestClient() { @@ -800,7 +805,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, const MediaConstraintsInterface* constraints, const PeerConnectionFactory::Options* options, rtc::scoped_ptr dtls_identity_store, - bool prefer_constraint_apis) { + bool prefer_constraint_apis, + rtc::Thread* worker_thread) { EXPECT_TRUE(!peer_connection_); EXPECT_TRUE(!peer_connection_factory_); if (!prefer_constraint_apis) { @@ -809,7 +815,7 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, prefer_constraint_apis_ = prefer_constraint_apis; rtc::scoped_ptr port_allocator( - new cricket::FakePortAllocator(rtc::Thread::Current(), nullptr)); + new cricket::FakePortAllocator(worker_thread, nullptr)); fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); if (fake_audio_capture_module_ == nullptr) { @@ -818,9 +824,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, fake_video_decoder_factory_ = new FakeWebRtcVideoDecoderFactory(); fake_video_encoder_factory_ = new FakeWebRtcVideoEncoderFactory(); peer_connection_factory_ = webrtc::CreatePeerConnectionFactory( - rtc::Thread::Current(), rtc::Thread::Current(), - fake_audio_capture_module_, fake_video_encoder_factory_, - fake_video_decoder_factory_); + worker_thread, rtc::Thread::Current(), fake_audio_capture_module_, + fake_video_encoder_factory_, fake_video_decoder_factory_); if (!peer_connection_factory_) { return false; } @@ -1019,7 +1024,9 @@ class P2PTestConductor : public testing::Test { P2PTestConductor() : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()) {} + ss_scope_(ss_.get()) { + RTC_CHECK(worker_thread_.Start()); + } bool SessionActive() { return initiating_client_->SessionActive() && @@ -1127,11 +1134,11 @@ class P2PTestConductor : public testing::Test { bool CreateTestClientsThatPreferNoConstraints() { initiating_client_.reset( - PeerConnectionTestClient::CreateClientPreferNoConstraints("Caller: ", - nullptr)); + PeerConnectionTestClient::CreateClientPreferNoConstraints( + "Caller: ", nullptr, &worker_thread_)); receiving_client_.reset( - PeerConnectionTestClient::CreateClientPreferNoConstraints("Callee: ", - nullptr)); + PeerConnectionTestClient::CreateClientPreferNoConstraints( + "Callee: ", nullptr, &worker_thread_)); if (!initiating_client_ || !receiving_client_) { return false; } @@ -1151,9 +1158,9 @@ class P2PTestConductor : public testing::Test { MediaConstraintsInterface* recv_constraints, PeerConnectionFactory::Options* recv_options) { initiating_client_.reset(PeerConnectionTestClient::CreateClient( - "Caller: ", init_constraints, init_options)); + "Caller: ", init_constraints, init_options, &worker_thread_)); receiving_client_.reset(PeerConnectionTestClient::CreateClient( - "Callee: ", recv_constraints, recv_options)); + "Callee: ", recv_constraints, recv_options, &worker_thread_)); if (!initiating_client_ || !receiving_client_) { return false; } @@ -1254,7 +1261,8 @@ 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_); + std::move(dtls_identity_store), prefer_constraint_apis_, + &worker_thread_); } void SendRtpData(webrtc::DataChannelInterface* dc, const std::string& data) { @@ -1294,6 +1302,9 @@ 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_;