Do not delete the turn port entry right away when the respective connection is deleted.

webrtc/p2p/base/turnport.cc

 /*
  *  Copyright 2012 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 122 matching lines @@
   enum BindState { STATE_UNBOUND, STATE_BINDING, STATE_BOUND };
   TurnEntry(TurnPort* port, int channel_id,
             const rtc::SocketAddress& ext_addr);
 
   TurnPort* port() { return port_; }
 
   int channel_id() const { return channel_id_; }
   const rtc::SocketAddress& address() const { return ext_addr_; }
   BindState state() const { return state_; }
 
+  uint32_t delayed_destroy_msg_id() { return delayed_destroy_msg_id_; }
+  void set_delayed_destroy_msg_id(uint32_t msg_id) {
+    delayed_destroy_msg_id_ = msg_id;
+  }

[pthatcher1, 2015/11/12 00:40:20]

As we discussed, let's try and make this scheduled_destruction_time_.

[honghaiz3, 2015/11/12 02:24:04]

When I tried to implement that, I found it became very difficult for testing. 
For example, in my current code, I can flush the invoker to process the queued
events. 
But if we check the timestamp, it won't take effect unless I actually waited for
that long. 
So using a message id may be better.

In fact, after second thought, I don't think there is any problem of using a
dynamic message ID, even though none of the existing code is doing it the same
way. message Id has a scope of the handler. In our case, it is the invoker. 
There should be no problem to create dynamic message id and use it. 
We can discuss more tomorrow. 

[pthatcher1, 2015/11/13 01:59:15]

I think we can get two birds with one stone here.  The two issues I have with
this CL are the dynamic message ID and the fact that we don't actually test the
timing.  I think we can fix both of these with one solution:  pass in a fake
clock into TurnPort.  We've been meaning to make a fake clock into unit tests
anyway to make them faster and less flaky.  This looks like a good fit.

We don't have a fake clock yet, but here's a CL that would add one:

https://webrtc-codereview.appspot.com/42599004/

I just never submitted it.



With a fake clock, we could do the following in the tests:

1.  Pass in the fake clock to TurnPort.
2.  Cause it to be scheduled for destruction
3.  Move the fake clock forward up to 1 second before the scheduled destruction
4.  Flush the thread events
5.  Verify that it wasn't destroyed
6.  Cause it to be scheduled for destruction again
7.  Move the fake clock forward up to 1 second after the scheduled destruction
8.  Flush the thread events
9.  Verify it was destroyed

I think that's a better test and better code and lays a groundwork for other
tests to use the fake clock. 

What do you think?



On 2015/11/12 02:24:04, honghaiz3 wrote:

[honghaiz3, 2015/11/13 22:44:15]

Revised as we discussed. We can avoid checking the time by using the timestamp
as a unique ID. 
we can use the fake clock for other tests later. 

+
   // Helper methods to send permission and channel bind requests.
   void SendCreatePermissionRequest();
   void SendChannelBindRequest(int delay);
   // Sends a packet to the given destination address.
   // This will wrap the packet in STUN if necessary.
   int Send(const void* data, size_t size, bool payload,
            const rtc::PacketOptions& options);
 
   void OnCreatePermissionSuccess();
   void OnCreatePermissionError(StunMessage* response, int code);
   void OnChannelBindSuccess();
   void OnChannelBindError(StunMessage* response, int code);
   // Signal sent when TurnEntry is destroyed.
   sigslot::signal1<TurnEntry*> SignalDestroyed;
 
  private:
   TurnPort* port_;
   int channel_id_;
   rtc::SocketAddress ext_addr_;
   BindState state_;
+  // A non-zero value indicates that this entry is scheduled to be destroyed
+  // with the following message ID, which can be used to cancel the scheduled
+  // destruction.
+  uint32_t delayed_destroy_msg_id_ = 0;
 };
 
 TurnPort::TurnPort(rtc::Thread* thread,
                    rtc::PacketSocketFactory* factory,
                    rtc::Network* network,
                    rtc::AsyncPacketSocket* socket,
                    const std::string& username,
                    const std::string& password,
                    const ProtocolAddress& server_address,
                    const RelayCredentials& credentials,
@@ skipping 59 matching lines @@
 
   // release the allocation by sending a refresh with
   // lifetime 0.
   if (ready()) {
     TurnRefreshRequest bye(this);
     bye.set_lifetime(0);
     SendRequest(&bye, 0);
   }
 
   while (!entries_.empty()) {
-    DestroyEntry(entries_.front()->address());
+    DestroyEntry(entries_.front());
   }
   if (resolver_) {
     resolver_->Destroy(false);
   }
   if (!SharedSocket()) {
     delete socket_;
   }
 }
 
 rtc::SocketAddress TurnPort::GetLocalAddress() const {
@@ skipping 178 matching lines @@
 
   if (!IsCompatibleAddress(address.address())) {
     return NULL;
   }
 
   if (state_ == STATE_DISCONNECTED) {
     return NULL;
   }
 
   // Create an entry, if needed, so we can get our permissions set up correctly.
-  CreateEntry(address.address());
+  GetOrCreateEntry(address.address());
 
   // A TURN port will have two candiates, STUN and TURN. STUN may not
   // present in all cases. If present stun candidate will be added first
   // and TURN candidate later.
   for (size_t index = 0; index < Candidates().size(); ++index) {
     if (Candidates()[index].type() == RELAY_PORT_TYPE) {
       ProxyConnection* conn = new ProxyConnection(this, index, address);
       conn->SignalDestroyed.connect(this, &TurnPort::OnConnectionDestroyed);
       AddConnection(conn);
       return conn;
@@ skipping 254 matching lines @@
       // with the alternate server. PrepareAddress will send stun binding once
       // the new socket is connected.
       ASSERT(server_address().proto == PROTO_TCP);
       ASSERT(!SharedSocket());
       delete socket_;
       socket_ = NULL;
       PrepareAddress();
     }
     return;
   }
-
   Port::OnMessage(message);
 }
 
 void TurnPort::OnAllocateRequestTimeout() {
   OnAllocateError();
 }
 
 void TurnPort::HandleDataIndication(const char* data, size_t size,
                                     const rtc::PacketTime& packet_time) {
   // Read in the message, and process according to RFC5766, Section 10.4.
@@ skipping 164 matching lines @@
 
 static bool MatchesChannelId(TurnEntry* e, int id) {
   return e->channel_id() == id;
 }
 TurnEntry* TurnPort::FindEntry(int channel_id) const {
   EntryList::const_iterator it = std::find_if(entries_.begin(), entries_.end(),
       std::bind2nd(std::ptr_fun(MatchesChannelId), channel_id));
   return (it != entries_.end()) ? *it : NULL;
 }
 
-TurnEntry* TurnPort::CreateEntry(const rtc::SocketAddress& addr) {
-  ASSERT(FindEntry(addr) == NULL);
-  TurnEntry* entry = new TurnEntry(this, next_channel_number_++, addr);
-  entries_.push_back(entry);
+TurnEntry* TurnPort::GetOrCreateEntry(const rtc::SocketAddress& addr) {
+  TurnEntry* entry = FindEntry(addr);
+  if (entry == nullptr) {
+    entry = new TurnEntry(this, next_channel_number_++, addr);
+    entries_.push_back(entry);
+  } else {
+    CancelEntryDestruction(entry);
+  }
   return entry;
 }
 
-void TurnPort::DestroyEntry(const rtc::SocketAddress& addr) {
-  TurnEntry* entry = FindEntry(addr);
+void TurnPort::DestroyEntry(TurnEntry* entry) {
   ASSERT(entry != NULL);
   entry->SignalDestroyed(entry);
   entries_.remove(entry);
   delete entry;
 }
 
 void TurnPort::OnConnectionDestroyed(Connection* conn) {
   // Destroying TurnEntry for the connection, which is already destroyed.
-  DestroyEntry(conn->remote_candidate().address());
+  rtc::SocketAddress remote_address = conn->remote_candidate().address();
+  TurnEntry* entry = FindEntry(remote_address);
+  ASSERT(entry != NULL);
+  ScheduleEntryDestruction(entry);
+}
+
+void TurnPort::ScheduleEntryDestruction(TurnEntry* entry) {
+  ASSERT(entry->delayed_destroy_msg_id() == 0);
+  ++last_destroy_entry_msg_id_;
+  entry->set_delayed_destroy_msg_id(last_destroy_entry_msg_id_);
+  invoker_.AsyncInvokeDelayed<void>(
+      thread(), rtc::Bind(&TurnPort::DestroyEntry, this, entry),
+      TURN_PERMISSION_TIMEOUT, last_destroy_entry_msg_id_);
+}
+
+void TurnPort::CancelEntryDestruction(TurnEntry* entry) {
+  ASSERT(entry->delayed_destroy_msg_id() != 0);
+  thread()->Clear(&invoker_, entry->delayed_destroy_msg_id());
+  entry->set_delayed_destroy_msg_id(0);
 }
 
 TurnAllocateRequest::TurnAllocateRequest(TurnPort* port)
     : StunRequest(new TurnMessage()),
       port_(port) {
 }
 
 void TurnAllocateRequest::Prepare(StunMessage* request) {
   // Create the request as indicated in RFC 5766, Section 6.1.
   request->SetType(TURN_ALLOCATE_REQUEST);
@@ skipping 433 matching lines @@
   // bind request as per http://tools.ietf.org/html/rfc5766#section-11.3
   if (code == STUN_ERROR_STALE_NONCE) {
     if (port_->UpdateNonce(response)) {
       // Send channel bind request with fresh nonce.
       SendChannelBindRequest(0);
     }
   }
 }
 
 }  // namespace cricket