Keep on sending stun binding requests on zero-cost networks.

webrtc/p2p/base/stunport.cc

Index: webrtc/p2p/base/stunport.cc
diff --git a/webrtc/p2p/base/stunport.cc b/webrtc/p2p/base/stunport.cc
index 8f37dd5218bb745b5e06ab6d8b2574faefc9aa44..723bb345c5ac95efc5b1ee746239d1ea2f808ead 100644
--- a/webrtc/p2p/base/stunport.cc
+++ b/webrtc/p2p/base/stunport.cc
@@ -23,22 +23,24 @@
 namespace cricket {
 
 // TODO: Move these to a common place (used in relayport too)
-const int KEEPALIVE_DELAY = 10 * 1000;  // 10 seconds - sort timeouts
-const int RETRY_TIMEOUT = 50 * 1000;    // ICE says 50 secs
-// Stop sending STUN binding requests after this amount of time
-// (in milliseconds) because the connection binding requests should keep
-// the NAT binding alive.
-const int KEEP_ALIVE_TIMEOUT = 2 * 60 * 1000;  // 2 minutes
+const int KEEPALIVE_DELAY_MS = 10 * 1000;   // 10 seconds - sort timeouts
+const int RETRY_TIMEOUT_US = 50 * 1000000;  // 50 seconds
+// Lifetime chosen for STUN ports on low-cost networks.
+const int INFINITE_LIFETIME = -1;
+// Lifetime for STUN ports on high-cost networks: 2 minutes
+const int32_t HIGH_COST_PORT_KEEPALIVE_LIFETIME_MS = 2 * 60 * 1000;
 
 // Handles a binding request sent to the STUN server.
 class StunBindingRequest : public StunRequest {
  public:
   StunBindingRequest(UDPPort* port,
                      const rtc::SocketAddress& addr,
-                     uint32_t deadline)
-      : port_(port), server_addr_(addr), deadline_(deadline) {
-    start_time_ = rtc::Time();
-  }
+                     uint64_t start_time_us,
+                     int64_t lifetime_us)
+      : port_(port),
+        server_addr_(addr),
+        start_time_us_(start_time_us),
+        lifetime_us_(lifetime_us) {}
 
   virtual ~StunBindingRequest() {
   }
@@ -62,11 +64,11 @@ class StunBindingRequest : public StunRequest {
       port_->OnStunBindingRequestSucceeded(server_addr_, addr);
     }
 
-    // We will do a keep-alive regardless of whether this request succeeds.
-    // It will be stopped after |deadline_| mostly to conserve the battery life.
-    if (rtc::Time() <= deadline_) {
+    // The keep-alive requests will be stopped after its lifetime has passed.
+    if (WithinLifetime(rtc::TimeMicros())) {
       port_->requests_.SendDelayed(
-          new StunBindingRequest(port_, server_addr_, deadline_),
+          new StunBindingRequest(port_, server_addr_, start_time_us_,
+                                 lifetime_us_),
           port_->stun_keepalive_delay());
     }
   }
@@ -77,34 +79,40 @@ class StunBindingRequest : public StunRequest {
       LOG(LS_ERROR) << "Bad allocate response error code";
     } else {
       LOG(LS_ERROR) << "Binding error response:"
-                 << " class=" << attr->eclass()
-                 << " number=" << attr->number()
-                 << " reason='" << attr->reason() << "'";
+                    << " class=" << attr->eclass()
+                    << " number=" << attr->number() << " reason='"
+                    << attr->reason() << "'";
     }
 
     port_->OnStunBindingOrResolveRequestFailed(server_addr_);
 
-    uint32_t now = rtc::Time();
-    if (now <= deadline_ && rtc::TimeDiff(now, start_time_) <= RETRY_TIMEOUT) {
+    uint64_t now = rtc::TimeMicros();
+    if (WithinLifetime(now) && now - start_time_us_ <= RETRY_TIMEOUT_US) {
       port_->requests_.SendDelayed(
-          new StunBindingRequest(port_, server_addr_, deadline_),
+          new StunBindingRequest(port_, server_addr_, start_time_us_,
+                                 lifetime_us_),
           port_->stun_keepalive_delay());
     }
   }
-
   virtual void OnTimeout() override {
     LOG(LS_ERROR) << "Binding request timed out from "
-      << port_->GetLocalAddress().ToSensitiveString()
-      << " (" << port_->Network()->name() << ")";
+                  << port_->GetLocalAddress().ToSensitiveString() << " ("
+                  << port_->Network()->name() << ")";
 
     port_->OnStunBindingOrResolveRequestFailed(server_addr_);
   }
 
  private:
+  // Returns true if |now| is within the lifetime of the request (a negative
+  // lifetime means infinite).
+  bool WithinLifetime(uint64_t now) const {
+    return lifetime_us_ < 0 || now <= start_time_us_ + lifetime_us_;

[honghaiz3, 2016/03/02 20:20:41]

Used the negative check instead of exactly -1 so that if the lifetime in
udp_port is -1, the lifetime in the request can be set just as -1 * 1000. 

+  }
   UDPPort* port_;
   const rtc::SocketAddress server_addr_;
-  uint32_t start_time_;
-  uint32_t deadline_;
+  uint64_t start_time_us_;
+  // The time duration for which this request will be rescheduled.
+  int64_t lifetime_us_;
 };
 
 UDPPort::AddressResolver::AddressResolver(
@@ -178,7 +186,7 @@ UDPPort::UDPPort(rtc::Thread* thread,
       socket_(socket),
       error_(0),
       ready_(false),
-      stun_keepalive_delay_(KEEPALIVE_DELAY),
+      stun_keepalive_delay_(KEEPALIVE_DELAY_MS),
       emit_local_for_anyaddress_(emit_local_for_anyaddress) {
   requests_.set_origin(origin);
 }
@@ -206,12 +214,18 @@ UDPPort::UDPPort(rtc::Thread* thread,
       socket_(NULL),
       error_(0),
       ready_(false),
-      stun_keepalive_delay_(KEEPALIVE_DELAY),
+      stun_keepalive_delay_(KEEPALIVE_DELAY_MS),
       emit_local_for_anyaddress_(emit_local_for_anyaddress) {
   requests_.set_origin(origin);
 }
 
 bool UDPPort::Init() {
+  // If this is a zero-cost network, it will keep on sending STUN binding
+  // requests indefinitely to keep the NAT binding alive. Otherwise, stop
+  // sending STUN binding requests after HIGH_COST_PORT_KEEPALIVE_LIFETIME_MS.
+  stun_keepalive_lifetime_ = (network_cost() == 0)
+                                 ? INFINITE_LIFETIME
+                                 : HIGH_COST_PORT_KEEPALIVE_LIFETIME_MS;
   if (!SharedSocket()) {
     ASSERT(socket_ == NULL);
     socket_ = socket_factory()->CreateUdpSocket(
@@ -397,8 +411,10 @@ void UDPPort::SendStunBindingRequest(const rtc::SocketAddress& stun_addr) {
   } else if (socket_->GetState() == rtc::AsyncPacketSocket::STATE_BOUND) {
     // Check if |server_addr_| is compatible with the port's ip.
     if (IsCompatibleAddress(stun_addr)) {
-      requests_.Send(new StunBindingRequest(this, stun_addr,
-                                            rtc::Time() + KEEP_ALIVE_TIMEOUT));
+      // The |stun_keepalive_lifetime_| is in milliseconds while the lifetime
+      // in the StunBindingRequest is in microseconds.

[pthatcher1, 2016/03/02 20:25:59]

Can we just make everything in this file milliseconds?  It's confusing having
both.

[honghaiz3, 2016/03/02 23:22:51]

It is hard to all use milliseconds or microseconds. 
The keep_alive_delay is used to schedule the next event, which has to be
milliseconds. 
The keep_alive_lifetime has to be in microseconds to avoid time wraparound. 
What I am doing now is 
Everything in UDPPort uses milliseconds. This also saves a few bytes. 
Everything in StunBindingRequest uses microseconds.

+      requests_.Send(new StunBindingRequest(this, stun_addr, rtc::TimeMicros(),
+                                            stun_keepalive_lifetime_ * 1000));
     } else {
       // Since we can't send stun messages to the server, we should mark this
       // port ready.