From b6d69282f563a3aa84a06ea20cfc133374c50a18 Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Wed, 10 Sep 2014 16:31:34 +0000 Subject: [PATCH] Enable shared socket for TurnPort. In AllocationSequence::OnReadPacket, we now hand the packet to both the TurnPort and StunPort if the remote address matches the server address. TESTED=AppRtc loopback call generates both turn and stun candidates. BUG=1746 R=juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/19189004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7138 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/p2p/base/portallocator.h | 2 - talk/p2p/base/stunport.h | 2 +- talk/p2p/client/basicportallocator.cc | 87 +++++++---------- talk/p2p/client/portallocator_unittest.cc | 113 ++++++++++++++-------- 4 files changed, 109 insertions(+), 95 deletions(-) diff --git a/talk/p2p/base/portallocator.h b/talk/p2p/base/portallocator.h index a3ff031b5..5bc389ed7 100644 --- a/talk/p2p/base/portallocator.h +++ b/talk/p2p/base/portallocator.h @@ -55,7 +55,6 @@ enum { PORTALLOCATOR_ENABLE_SHARED_UFRAG = 0x80, PORTALLOCATOR_ENABLE_SHARED_SOCKET = 0x100, PORTALLOCATOR_ENABLE_STUN_RETRANSMIT_ATTRIBUTE = 0x200, - PORTALLOCATOR_ENABLE_TURN_SHARED_SOCKET = 0x400, }; const uint32 kDefaultPortAllocatorFlags = 0; @@ -170,7 +169,6 @@ class PortAllocator : public sigslot::has_slots<> { uint32 step_delay() const { return step_delay_; } void set_step_delay(uint32 delay) { - ASSERT(delay >= kMinimumStepDelay); step_delay_ = delay; } diff --git a/talk/p2p/base/stunport.h b/talk/p2p/base/stunport.h index 0a49b67d2..b3b6d5b1c 100644 --- a/talk/p2p/base/stunport.h +++ b/talk/p2p/base/stunport.h @@ -82,7 +82,7 @@ class UDPPort : public Port { return socket_->GetLocalAddress(); } - const ServerAddresses server_addresses() const { + const ServerAddresses& server_addresses() const { return server_addresses_; } void diff --git a/talk/p2p/client/basicportallocator.cc b/talk/p2p/client/basicportallocator.cc index 6c184b469..eda0b4b49 100644 --- a/talk/p2p/client/basicportallocator.cc +++ b/talk/p2p/client/basicportallocator.cc @@ -149,9 +149,6 @@ class AllocationSequence : public rtc::MessageHandler, const rtc::PacketTime& packet_time); void OnPortDestroyed(PortInterface* port); - void OnResolvedTurnServerAddress( - TurnPort* port, const rtc::SocketAddress& server_address, - const rtc::SocketAddress& resolved_server_address); BasicPortAllocatorSession* session_; rtc::Network* network_; @@ -163,8 +160,7 @@ class AllocationSequence : public rtc::MessageHandler, rtc::scoped_ptr udp_socket_; // There will be only one udp port per AllocationSequence. UDPPort* udp_port_; - // Keeping a map for turn ports keyed with server addresses. - std::map turn_ports_; + std::vector turn_ports_; int phase_; }; @@ -1054,26 +1050,15 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { // don't pass shared socket for ports which will create TCP sockets. // TODO(mallinath) - Enable shared socket mode for TURN ports. Disabled // due to webrtc bug https://code.google.com/p/webrtc/issues/detail?id=3537 - if (IsFlagSet(PORTALLOCATOR_ENABLE_TURN_SHARED_SOCKET) && + if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET) && relay_port->proto == PROTO_UDP) { port = TurnPort::Create(session_->network_thread(), session_->socket_factory(), network_, udp_socket_.get(), session_->username(), session_->password(), *relay_port, config.credentials, config.priority); - // If we are using shared socket for TURN and udp ports, we need to - // find a way to demux the packets to the correct port when received. - // Mapping against server_address is one way of doing this. When packet - // is received the remote_address will be checked against the map. - // If server address is not resolved, a signal will be sent from the port - // after the address is resolved. The map entry will updated with the - // resolved address when the signal is received from the port. - if ((*relay_port).address.IsUnresolved()) { - // If server address is not resolved then listen for signal from port. - port->SignalResolvedServerAddress.connect( - this, &AllocationSequence::OnResolvedTurnServerAddress); - } - turn_ports_[(*relay_port).address] = port; + + turn_ports_.push_back(port); // Listen to the port destroyed signal, to allow AllocationSequence to // remove entrt from it's map. port->SignalDestroyed.connect(this, &AllocationSequence::OnPortDestroyed); @@ -1097,51 +1082,45 @@ void AllocationSequence::OnReadPacket( const rtc::SocketAddress& remote_addr, const rtc::PacketTime& packet_time) { ASSERT(socket == udp_socket_.get()); - // If the packet is received from one of the TURN server in the config, then - // pass down the packet to that port, otherwise it will be handed down to - // the local udp port. - Port* port = NULL; - std::map::iterator iter = - turn_ports_.find(remote_addr); - if (iter != turn_ports_.end()) { - port = iter->second; - } else if (udp_port_) { - port = udp_port_; + + bool turn_port_found = false; + + // Try to find the TurnPort that matches the remote address. Note that the + // message could be a STUN binding response if the TURN server is also used as + // a STUN server. We don't want to parse every message here to check if it is + // a STUN binding response, so we pass the message to TurnPort regardless of + // the message type. The TurnPort will just ignore the message since it will + // not find any request by transaction ID. + for (std::vector::const_iterator it = turn_ports_.begin(); + it != turn_ports_.end(); ++it) { + TurnPort* port = *it; + if (port->server_address().address == remote_addr) { + port->HandleIncomingPacket(socket, data, size, remote_addr, packet_time); + turn_port_found = true; + break; + } } - ASSERT(port != NULL); - if (port) { - port->HandleIncomingPacket(socket, data, size, remote_addr, packet_time); + + if (udp_port_) { + const ServerAddresses& stun_servers = udp_port_->server_addresses(); + + // Pass the packet to the UdpPort if there is no matching TurnPort, or if + // the TURN server is also a STUN server. + if (!turn_port_found || + stun_servers.find(remote_addr) != stun_servers.end()) { + udp_port_->HandleIncomingPacket( + socket, data, size, remote_addr, packet_time); + } } } void AllocationSequence::OnPortDestroyed(PortInterface* port) { if (udp_port_ == port) { udp_port_ = NULL; - } else { - std::map::iterator iter; - for (iter = turn_ports_.begin(); iter != turn_ports_.end(); ++iter) { - if (iter->second == port) { - turn_ports_.erase(iter); - break; - } - } - } -} - -void AllocationSequence::OnResolvedTurnServerAddress( - TurnPort* port, const rtc::SocketAddress& server_address, - const rtc::SocketAddress& resolved_server_address) { - std::map::iterator iter; - iter = turn_ports_.find(server_address); - if (iter == turn_ports_.end()) { - LOG(LS_INFO) << "TurnPort entry is not found in the map."; return; } - ASSERT(iter->second == port); - // Remove old entry and then insert using the resolved address as key. - turn_ports_.erase(iter); - turn_ports_[resolved_server_address] = port; + turn_ports_.erase(std::find(turn_ports_.begin(), turn_ports_.end(), port)); } // PortConfiguration diff --git a/talk/p2p/client/portallocator_unittest.cc b/talk/p2p/client/portallocator_unittest.cc index c3830320e..e793064e5 100644 --- a/talk/p2p/client/portallocator_unittest.cc +++ b/talk/p2p/client/portallocator_unittest.cc @@ -133,9 +133,32 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { bool SetPortRange(int min_port, int max_port) { return allocator_->SetPortRange(min_port, max_port); } - rtc::NATServer* CreateNatServer(const SocketAddress& addr, - rtc::NATType type) { - return new rtc::NATServer(type, vss_.get(), addr, vss_.get(), addr); + void ResetWithNatServer(const rtc::SocketAddress& stun_server) { + nat_server_.reset(new rtc::NATServer( + rtc::NAT_OPEN_CONE, vss_.get(), kNatAddr, vss_.get(), kNatAddr)); + + ServerAddresses stun_servers; + stun_servers.insert(stun_server); + allocator_.reset(new cricket::BasicPortAllocator( + &network_manager_, &nat_socket_factory_, stun_servers)); + allocator().set_step_delay(cricket::kMinimumStepDelay); + } + + void AddTurnServers(const rtc::SocketAddress& udp_turn, + const rtc::SocketAddress& tcp_turn) { + cricket::RelayServerConfig relay_server(cricket::RELAY_TURN); + cricket::RelayCredentials credentials(kTurnUsername, kTurnPassword); + relay_server.credentials = credentials; + + if (!udp_turn.IsNil()) { + relay_server.ports.push_back(cricket::ProtocolAddress( + kTurnUdpIntAddr, cricket::PROTO_UDP, false)); + } + if (!tcp_turn.IsNil()) { + relay_server.ports.push_back(cricket::ProtocolAddress( + kTurnTcpIntAddr, cricket::PROTO_TCP, false)); + } + allocator_->AddRelay(relay_server); } bool CreateSession(int component) { @@ -254,6 +277,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { rtc::scoped_ptr vss_; rtc::scoped_ptr fss_; rtc::SocketServerScope ss_scope_; + rtc::scoped_ptr nat_server_; rtc::NATSocketFactory nat_factory_; rtc::BasicPacketSocketFactory nat_socket_factory_; cricket::TestStunServer stun_server_; @@ -580,13 +604,8 @@ TEST_F(PortAllocatorTest, TestCandidateFilterWithHostOnly) { // Host is behind the NAT. TEST_F(PortAllocatorTest, TestCandidateFilterWithReflexiveOnly) { AddInterface(kPrivateAddr); - rtc::scoped_ptr nat_server( - CreateNatServer(kNatAddr, rtc::NAT_OPEN_CONE)); - ServerAddresses stun_servers; - stun_servers.insert(kStunAddr); - allocator_.reset(new cricket::BasicPortAllocator( - &network_manager_, &nat_socket_factory_, stun_servers)); - allocator().set_step_delay(cricket::kMinimumStepDelay); + ResetWithNatServer(kStunAddr); + allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); allocator().set_candidate_filter(cricket::CF_REFLEXIVE); @@ -771,13 +790,8 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithoutNat) { // local candidates as client behind a nat. TEST_F(PortAllocatorTest, TestSharedSocketWithNat) { AddInterface(kClientAddr); - rtc::scoped_ptr nat_server( - CreateNatServer(kNatAddr, rtc::NAT_OPEN_CONE)); - ServerAddresses stun_servers; - stun_servers.insert(kStunAddr); - allocator_.reset(new cricket::BasicPortAllocator( - &network_manager_, &nat_socket_factory_, stun_servers)); - allocator_->set_step_delay(cricket::kMinimumStepDelay); + ResetWithNatServer(kStunAddr); + allocator_->set_flags(allocator().flags() | cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); @@ -799,14 +813,8 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP); AddInterface(kClientAddr); allocator_.reset(new cricket::BasicPortAllocator(&network_manager_)); - cricket::RelayServerConfig relay_server(cricket::RELAY_TURN); - cricket::RelayCredentials credentials(kTurnUsername, kTurnPassword); - relay_server.credentials = credentials; - relay_server.ports.push_back(cricket::ProtocolAddress( - kTurnUdpIntAddr, cricket::PROTO_UDP, false)); - relay_server.ports.push_back(cricket::ProtocolAddress( - kTurnTcpIntAddr, cricket::PROTO_TCP, false)); - allocator_->AddRelay(relay_server); + + AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); allocator_->set_step_delay(cricket::kMinimumStepDelay); allocator_->set_flags(allocator().flags() | @@ -863,20 +871,10 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithServerAddressResolve) { // stun and turn candidates. TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurn) { AddInterface(kClientAddr); - rtc::scoped_ptr nat_server( - CreateNatServer(kNatAddr, rtc::NAT_OPEN_CONE)); - ServerAddresses stun_servers; - stun_servers.insert(kStunAddr); - allocator_.reset(new cricket::BasicPortAllocator( - &network_manager_, &nat_socket_factory_, stun_servers)); - cricket::RelayServerConfig relay_server(cricket::RELAY_TURN); - cricket::RelayCredentials credentials(kTurnUsername, kTurnPassword); - relay_server.credentials = credentials; - relay_server.ports.push_back(cricket::ProtocolAddress( - kTurnUdpIntAddr, cricket::PROTO_UDP, false)); - allocator_->AddRelay(relay_server); + ResetWithNatServer(kStunAddr); + + AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); - allocator_->set_step_delay(cricket::kMinimumStepDelay); allocator_->set_flags(allocator().flags() | cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET | @@ -902,6 +900,45 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurn) { EXPECT_EQ(1U, ports_[1]->Candidates().size()); } +// Test that when PORTALLOCATOR_ENABLE_SHARED_SOCKET is enabled and the TURN +// server is also used as the STUN server, we should get 'local', 'stun', and +// 'relay' candidates. +TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurnAsStun) { + AddInterface(kClientAddr); + ResetWithNatServer(kTurnUdpIntAddr); + AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); + + // Must set the step delay to 0 to make sure the relay allocation phase is + // started before the STUN candidates are obtained, so that the STUN binding + // response is processed when both StunPort and TurnPort exist to reproduce + // webrtc issue 3537. + allocator_->set_step_delay(0); + allocator_->set_flags(allocator().flags() | + cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | + cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET | + cricket::PORTALLOCATOR_DISABLE_TCP); + + EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + + ASSERT_EQ_WAIT(3U, candidates_.size(), kDefaultAllocationTimeout); + EXPECT_PRED5(CheckCandidate, candidates_[0], + cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr); + EXPECT_PRED5(CheckCandidate, candidates_[1], + cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp", + rtc::SocketAddress(kNatAddr.ipaddr(), 0)); + EXPECT_PRED5(CheckCandidate, candidates_[2], + cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp", + rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); + EXPECT_EQ(candidates_[2].related_address(), candidates_[1].address()); + + EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); + EXPECT_EQ(3U, candidates_.size()); + // Local port will be created first and then TURN port. + EXPECT_EQ(2U, ports_[0]->Candidates().size()); + EXPECT_EQ(1U, ports_[1]->Candidates().size()); +} + // This test verifies when PORTALLOCATOR_ENABLE_SHARED_SOCKET flag is enabled // and fail to generate STUN candidate, local UDP candidate is generated // properly.