From 1590c3937c77ebe878a04142eceee7e330d4fe38 Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Tue, 24 May 2016 13:15:02 -0700 Subject: [PATCH] Fire a signal when the transport channel state changes This fixes an issue that sometimes the transport channel state changes but the transportcontroller is not notified. BUG=5907 R=deadbeef@webrtc.org, pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1996693002 . Cr-Commit-Position: refs/heads/master@{#12880} --- webrtc/p2p/base/dtlstransportchannel.cc | 8 +++---- webrtc/p2p/base/dtlstransportchannel.h | 2 +- webrtc/p2p/base/faketransportcontroller.h | 4 +++- webrtc/p2p/base/p2ptransportchannel.cc | 12 +++++----- .../p2p/base/p2ptransportchannel_unittest.cc | 22 +++++++++++++++++++ webrtc/p2p/base/port.cc | 2 +- webrtc/p2p/base/transportchannelimpl.h | 5 ++--- webrtc/p2p/base/transportcontroller.cc | 8 +++---- webrtc/p2p/base/transportcontroller.h | 2 +- webrtc/p2p/quic/quictransportchannel.cc | 9 ++++---- webrtc/p2p/quic/quictransportchannel.h | 2 +- 11 files changed, 51 insertions(+), 25 deletions(-) diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index 1e3df13493..e34dfbd3e2 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -129,8 +129,8 @@ DtlsTransportChannelWrapper::DtlsTransportChannelWrapper( &DtlsTransportChannelWrapper::OnRouteChange); channel_->SignalSelectedCandidatePairChanged.connect( this, &DtlsTransportChannelWrapper::OnSelectedCandidatePairChanged); - channel_->SignalConnectionRemoved.connect(this, - &DtlsTransportChannelWrapper::OnConnectionRemoved); + channel_->SignalStateChanged.connect( + this, &DtlsTransportChannelWrapper::OnChannelStateChanged); channel_->SignalReceivingState.connect(this, &DtlsTransportChannelWrapper::OnReceivingState); } @@ -671,10 +671,10 @@ void DtlsTransportChannelWrapper::OnSelectedCandidatePairChanged( last_sent_packet_id); } -void DtlsTransportChannelWrapper::OnConnectionRemoved( +void DtlsTransportChannelWrapper::OnChannelStateChanged( TransportChannelImpl* channel) { ASSERT(channel == channel_); - SignalConnectionRemoved(this); + SignalStateChanged(this); } void DtlsTransportChannelWrapper::Reconnect() { diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h index c5f55469f2..c17962a7ec 100644 --- a/webrtc/p2p/base/dtlstransportchannel.h +++ b/webrtc/p2p/base/dtlstransportchannel.h @@ -221,7 +221,7 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, int last_sent_packet_id); - void OnConnectionRemoved(TransportChannelImpl* channel); + void OnChannelStateChanged(TransportChannelImpl* channel); void Reconnect(); rtc::Thread* worker_thread_; // Everything should occur on this thread. diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index 321537dd59..7f0d9693ec 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -170,8 +170,10 @@ class FakeTransportChannel : public TransportChannelImpl, connection_count_ = connection_count; if (connection_count) had_connection_ = true; + // In this fake transport channel, |connection_count_| determines the + // transport channel state. if (connection_count_ < old_connection_count) - SignalConnectionRemoved(this); + SignalStateChanged(this); } void SetCandidatesGatheringComplete() { diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 2801c44646..92bec6f897 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -357,7 +357,6 @@ TransportChannelState P2PTransportChannel::ComputeState() const { } } - LOG_J(LS_VERBOSE, this) << "Ice is completed for this channel."; return TransportChannelState::STATE_COMPLETED; } @@ -1209,7 +1208,13 @@ void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) { // change, it should be called after all the connection states have changed. For // example, we call this at the end of SortConnections. void P2PTransportChannel::UpdateState() { - state_ = ComputeState(); + TransportChannelState state = ComputeState(); + if (state_ != state) { + LOG_J(LS_INFO, this) << "Transport channel state changed from " << state_ + << " to " << state; + state_ = state; + SignalStateChanged(this); + } bool writable = best_connection_ && best_connection_->writable(); set_writable(writable); @@ -1472,9 +1477,6 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) { } UpdateState(); - // SignalConnectionRemoved should be called after the channel state is - // updated because the receiver of the event may access the channel state. - SignalConnectionRemoved(this); } // When a port is destroyed remove it from our list of ports to use for diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index d645c57894..e2c5003126 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -2008,6 +2008,8 @@ class P2PTransportChannelPingTest : public testing::Test, this, &P2PTransportChannelPingTest::OnSelectedCandidatePairChanged); ch->SignalReadyToSend.connect(this, &P2PTransportChannelPingTest::OnReadyToSend); + ch->SignalStateChanged.connect( + this, &P2PTransportChannelPingTest::OnChannelStateChanged); } cricket::Candidate CreateHostCandidate(const std::string& ip, @@ -2077,6 +2079,9 @@ class P2PTransportChannelPingTest : public testing::Test, void OnReadyToSend(cricket::TransportChannel* channel) { channel_ready_to_send_ = true; } + void OnChannelStateChanged(cricket::TransportChannelImpl* channel) { + channel_state_ = channel->GetState(); + } cricket::CandidatePairInterface* last_selected_candidate_pair() { return last_selected_candidate_pair_; @@ -2084,6 +2089,7 @@ class P2PTransportChannelPingTest : public testing::Test, int last_sent_packet_id() { return last_sent_packet_id_; } bool channel_ready_to_send() { return channel_ready_to_send_; } void reset_channel_ready_to_send() { channel_ready_to_send_ = false; } + cricket::TransportChannelState channel_state() { return channel_state_; } private: std::unique_ptr pss_; @@ -2092,6 +2098,7 @@ class P2PTransportChannelPingTest : public testing::Test, cricket::CandidatePairInterface* last_selected_candidate_pair_ = nullptr; int last_sent_packet_id_ = -1; bool channel_ready_to_send_ = false; + cricket::TransportChannelState channel_state_ = cricket::STATE_INIT; }; TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { @@ -2145,6 +2152,21 @@ TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { EXPECT_EQ(conn2, FindNextPingableConnectionAndPingIt(&ch)); } +TEST_F(P2PTransportChannelPingTest, TestSignalStateChanged) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("state change", 1, &pa); + PrepareChannel(&ch); + ch.Connect(); + ch.MaybeStartGathering(); + ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1)); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + // Pruning the connection reduces the set of active connections and changes + // the channel state. + conn1->Prune(); + EXPECT_EQ_WAIT(cricket::STATE_FAILED, channel_state(), kDefaultTimeout); +} + // Test adding remote candidates with different ufrags. If a remote candidate // is added with an old ufrag, it will be discarded. If it is added with a // ufrag that was not seen before, it will be used to create connections diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 15da8f95ae..051d8c667d 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1059,7 +1059,7 @@ void Connection::OnReadyToSend() { void Connection::Prune() { if (!pruned_ || active()) { - LOG_J(LS_VERBOSE, this) << "Connection pruned"; + LOG_J(LS_INFO, this) << "Connection pruned"; pruned_ = true; requests_.Clear(); set_write_state(STATE_WRITE_TIMEOUT); diff --git a/webrtc/p2p/base/transportchannelimpl.h b/webrtc/p2p/base/transportchannelimpl.h index f548aa012f..70cb4e292f 100644 --- a/webrtc/p2p/base/transportchannelimpl.h +++ b/webrtc/p2p/base/transportchannelimpl.h @@ -100,9 +100,8 @@ class TransportChannelImpl : public TransportChannel { // agents. sigslot::signal1 SignalRoleConflict; - // Emitted whenever the number of connections available to the transport - // channel decreases. - sigslot::signal1 SignalConnectionRemoved; + // Emitted whenever the transport channel state changed. + sigslot::signal1 SignalStateChanged; private: RTC_DISALLOW_COPY_AND_ASSIGN(TransportChannelImpl); diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index a961541dc3..f40e5a3c41 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -177,8 +177,8 @@ TransportChannel* TransportController::CreateTransportChannel_n( this, &TransportController::OnChannelCandidatesRemoved_n); channel->SignalRoleConflict.connect( this, &TransportController::OnChannelRoleConflict_n); - channel->SignalConnectionRemoved.connect( - this, &TransportController::OnChannelConnectionRemoved_n); + channel->SignalStateChanged.connect( + this, &TransportController::OnChannelStateChanged_n); channels_.insert(channels_.end(), RefCountedChannel(channel))->AddRef(); // Adding a channel could cause aggregate state to change. UpdateAggregateStates_n(); @@ -592,12 +592,12 @@ void TransportController::OnChannelRoleConflict_n( } } -void TransportController::OnChannelConnectionRemoved_n( +void TransportController::OnChannelStateChanged_n( TransportChannelImpl* channel) { RTC_DCHECK(network_thread_->IsCurrent()); LOG(LS_INFO) << channel->transport_name() << " TransportChannel " << channel->component() - << " connection removed. Check if state is complete."; + << " state changed. Check if state is complete."; UpdateAggregateStates_n(); } diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h index cc16a768ab..9996d7235d 100644 --- a/webrtc/p2p/base/transportcontroller.h +++ b/webrtc/p2p/base/transportcontroller.h @@ -198,7 +198,7 @@ class TransportController : public sigslot::has_slots<>, void OnChannelCandidatesRemoved_n(TransportChannelImpl* channel, const Candidates& candidates); void OnChannelRoleConflict_n(TransportChannelImpl* channel); - void OnChannelConnectionRemoved_n(TransportChannelImpl* channel); + void OnChannelStateChanged_n(TransportChannelImpl* channel); void UpdateAggregateStates_n(); diff --git a/webrtc/p2p/quic/quictransportchannel.cc b/webrtc/p2p/quic/quictransportchannel.cc index 968faee7bd..6aafa9e3db 100644 --- a/webrtc/p2p/quic/quictransportchannel.cc +++ b/webrtc/p2p/quic/quictransportchannel.cc @@ -145,8 +145,8 @@ QuicTransportChannel::QuicTransportChannel(TransportChannelImpl* channel) &QuicTransportChannel::OnRouteChange); channel_->SignalSelectedCandidatePairChanged.connect( this, &QuicTransportChannel::OnSelectedCandidatePairChanged); - channel_->SignalConnectionRemoved.connect( - this, &QuicTransportChannel::OnConnectionRemoved); + channel_->SignalStateChanged.connect( + this, &QuicTransportChannel::OnChannelStateChanged); channel_->SignalReceivingState.connect( this, &QuicTransportChannel::OnReceivingState); @@ -401,9 +401,10 @@ void QuicTransportChannel::OnSelectedCandidatePairChanged( last_sent_packet_id); } -void QuicTransportChannel::OnConnectionRemoved(TransportChannelImpl* channel) { +void QuicTransportChannel::OnChannelStateChanged( + TransportChannelImpl* channel) { ASSERT(channel == channel_.get()); - SignalConnectionRemoved(this); + SignalStateChanged(this); } bool QuicTransportChannel::MaybeStartQuic() { diff --git a/webrtc/p2p/quic/quictransportchannel.h b/webrtc/p2p/quic/quictransportchannel.h index dec24d2154..688323f722 100644 --- a/webrtc/p2p/quic/quictransportchannel.h +++ b/webrtc/p2p/quic/quictransportchannel.h @@ -247,7 +247,7 @@ class QuicTransportChannel : public TransportChannelImpl, TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, int last_sent_packet_id); - void OnConnectionRemoved(TransportChannelImpl* channel); + void OnChannelStateChanged(TransportChannelImpl* channel); // Callbacks for |quic_|. // Called when |quic_| has established the crypto handshake.