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

Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections.

BUG=

Committed: https://crrev.com/e2af9ef638408d8ec0a9d0649f8ea86384fa4816
Cr-Commit-Position: refs/heads/master@{#11862}

[honghaiz3, 2016-02-25 21:48:56]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-02-25 22:06:26]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[honghaiz3, 2016-02-25 22:07:40]

Description was changed from

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

BUG=
==========

to

==========
Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections. 

BUG=
==========

[juberti2, 2016-02-26 01:53:40]

juberti@chromium.org changed reviewers:
+ juberti@chromium.org

[juberti2, 2016-02-26 01:53:41]

https://codereview.webrtc.org/1737493004/diff/20001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/20001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:403: uint32_t deadline = (network_cost() == 0)
I don't like the |deadline| terminology - it's really confusing what the
deadline applies to. Can we call this keepalive_duration (here and above)?

Also, this needs a comment explaining what this code is trying to do.

[honghaiz3, 2016-02-26 18:39:15]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2016-02-26 18:43:36]

Patchset #2 (id:60001) has been deleted

[honghaiz3, 2016-02-26 18:46:57]

https://codereview.webrtc.org/1737493004/diff/20001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/20001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:403: uint32_t deadline = (network_cost() == 0)
On 2016/02/26 01:53:41, juberti2 wrote:
> I don't like the |deadline| terminology - it's really confusing what the
> deadline applies to. Can we call this keepalive_duration (here and above)?
> 
> Also, this needs a comment explaining what this code is trying to do.
Added comments. 
Renamed it to keepalive_lifetime. I feel that duration may have the confusion
with the duration between two requests.
Also I realize that Time() may wrap around, so use a negative lifetime value to
indicate infinite lifetime.

[pthatcher1, 2016-02-26 22:33:52]

Mostly readability stuff.

Are you going to add unit tests?

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:28: const int KEEPALIVE_LIFETIME = 2 * 60 * 1000; 
// 2 minutes
This could use a comment explaining what it is.

Plus, isn't this now just value for high-cost networks?  So shouldn't it be:

HIGH_COST_STUN_PORT_KEEPALIVE_LIFETIME

?

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:86: if ((lifetime_ < 0 || rtc::TimeDiff(now,
start_time_) <= lifetime_) &&
It seems like we could use a helper method of BeyondLifetime(now).

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:404: int keepalive_lifetime = (network_cost() == 0)
? -1 : KEEPALIVE_LIFETIME;
I think we should make a constant for UNLIMITED_LIFETIME to make this a bit more
readable.

You could use it above also as "if (lifetime_ <= UNLIMITED_LIFETIME)"

[honghaiz3, 2016-02-29 19:25:00]

Patchset #3 (id:100001) has been deleted

[honghaiz3, 2016-02-29 19:30:21]

Patchset #3 (id:120001) has been deleted

[honghaiz3, 2016-02-29 19:34:11]

As the logic heavily depends on the realtime clock, it is hard to directly test
the difference between "the stun request will stop in 2 minutes" vs "it will
never stop"
I think a manual test is OK for this. Let me know if you have a different
thought.

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:28: const int KEEPALIVE_LIFETIME = 2 * 60 * 1000; 
// 2 minutes
On 2016/02/26 22:33:52, pthatcher1 wrote:
> This could use a comment explaining what it is.
> 
> Plus, isn't this now just value for high-cost networks?  So shouldn't it be:
> 
> HIGH_COST_STUN_PORT_KEEPALIVE_LIFETIME
> 
> ?

Done.

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:86: if ((lifetime_ < 0 || rtc::TimeDiff(now,
start_time_) <= lifetime_) &&
On 2016/02/26 22:33:52, pthatcher1 wrote:
> It seems like we could use a helper method of BeyondLifetime(now).

Done, although this will lose the benefit of not requiring to evaluate
rtc::Time() if lifetime is infinite.

https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
webrtc/p2p/base/stunport.cc:404: int keepalive_lifetime = (network_cost() == 0)
? -1 : KEEPALIVE_LIFETIME;
On 2016/02/26 22:33:52, pthatcher1 wrote:
> I think we should make a constant for UNLIMITED_LIFETIME to make this a bit
more
> readable.
> 
> You could use it above also as "if (lifetime_ <= UNLIMITED_LIFETIME)"

Done. Used INIFINTE_LIFETIME. Also the logic changes to if (lifetime_ ==
INFINITE_LIFETIME || ....), which Looks more readable.

[pthatcher1, 2016-03-01 22:59:05]

I think we could add two unit tests:

1.  Test that when the network cost == 0, the scheduled lifetime is indefinite,
and other is not.
2.  Test that the lifetime code is correct, so if we send a very short lifetime,
it doesn't send long, but for a longer one (if INFININTE), it does.


On 2016/02/29 19:34:11, honghaiz3 wrote:
> As the logic heavily depends on the realtime clock, it is hard to directly
test
> the difference between "the stun request will stop in 2 minutes" vs "it will
> never stop"
> I think a manual test is OK for this. Let me know if you have a different
> thought.
> 
>
https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport.cc
> File webrtc/p2p/base/stunport.cc (right):
> 
>
https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
> webrtc/p2p/base/stunport.cc:28: const int KEEPALIVE_LIFETIME = 2 * 60 * 1000; 
> // 2 minutes
> On 2016/02/26 22:33:52, pthatcher1 wrote:
> > This could use a comment explaining what it is.
> > 
> > Plus, isn't this now just value for high-cost networks?  So shouldn't it be:
> > 
> > HIGH_COST_STUN_PORT_KEEPALIVE_LIFETIME
> > 
> > ?
> 
> Done.
> 
>
https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
> webrtc/p2p/base/stunport.cc:86: if ((lifetime_ < 0 || rtc::TimeDiff(now,
> start_time_) <= lifetime_) &&
> On 2016/02/26 22:33:52, pthatcher1 wrote:
> > It seems like we could use a helper method of BeyondLifetime(now).
> 
> Done, although this will lose the benefit of not requiring to evaluate
> rtc::Time() if lifetime is infinite.
> 
>
https://codereview.webrtc.org/1737493004/diff/80001/webrtc/p2p/base/stunport....
> webrtc/p2p/base/stunport.cc:404: int keepalive_lifetime = (network_cost() ==
0)
> ? -1 : KEEPALIVE_LIFETIME;
> On 2016/02/26 22:33:52, pthatcher1 wrote:
> > I think we should make a constant for UNLIMITED_LIFETIME to make this a bit
> more
> > readable.
> > 
> > You could use it above also as "if (lifetime_ <= UNLIMITED_LIFETIME)"
> 
> Done. Used INIFINTE_LIFETIME. Also the logic changes to if (lifetime_ ==
> INFINITE_LIFETIME || ....), which Looks more readable.

[honghaiz3, 2016-03-02 18:44:35]

Patchset #4 (id:160001) has been deleted

[honghaiz3, 2016-03-02 18:48:00]

Added tests and made some changes for the tests, also changed the lifetime and
start_time to 64 bits to have less chance of integer overflow. PTAL.

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

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:109: return lifetime_us_ < 0 || now <=
start_time_us_ + lifetime_us_;
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.

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

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:415: // in the StunBindingRequest is in
microseconds.
Can we just make everything in this file milliseconds?  It's confusing having
both.

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport.h
File webrtc/p2p/base/stunport.h (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.h:115: bool RequestExists(int msg_type) { return
requests_.Exist(msg_type); }
A better name might be HasPendingRequest

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunrequ...
File webrtc/p2p/base/stunrequest.cc (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunrequ...
webrtc/p2p/base/stunrequest.cc:66: bool StunRequestManager::Exist(int msg_type)
{
I think HasAny might be a better name.

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

Thanks! PTAL.

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:415: // in the StunBindingRequest is in
microseconds.
On 2016/03/02 20:25:59, pthatcher1 wrote:
> Can we just make everything in this file milliseconds?  It's confusing having
> both.
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.

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport.h
File webrtc/p2p/base/stunport.h (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.h:115: bool RequestExists(int msg_type) { return
requests_.Exist(msg_type); }
On 2016/03/02 20:25:59, pthatcher1 wrote:
> A better name might be HasPendingRequest

Done.

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunrequ...
File webrtc/p2p/base/stunrequest.cc (right):

https://codereview.webrtc.org/1737493004/diff/180001/webrtc/p2p/base/stunrequ...
webrtc/p2p/base/stunrequest.cc:66: bool StunRequestManager::Exist(int msg_type)
{
On 2016/03/02 20:25:59, pthatcher1 wrote:
> I think HasAny might be a better name.

Done. using HasRequest

[honghaiz3, 2016-03-02 23:26:07]

Patchset #5 (id:200001) has been deleted

[honghaiz3, 2016-03-03 00:03:45]

Change to 32-bit time for now. 
Leave the time-value fix at a separate CL.

[pthatcher1, 2016-03-03 00:57:28]

lgtm

[honghaiz3, 2016-03-03 01:00:30]

Patchset #6 (id:240001) has been deleted

[honghaiz3, 2016-03-03 01:00:42]

Patchset #6 (id:260001) has been deleted

[honghaiz3, 2016-03-03 01:00:51]

Patchset #7 (id:300001) has been deleted

[honghaiz3, 2016-03-03 01:02:58]

Patchset #6 (id:280001) has been deleted

[honghaiz3, 2016-03-03 01:05:19]

As I think more about it, it is OK to use TimeDiff even with the current
"efficient implementation" as long as the elapsed time does not overflow. The
only catch is that we should not try to compare two timestamps directly. Need to
use TimeDiff or TimeIsLaterOrEqual.

[honghaiz3, 2016-03-03 01:07:20]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-03 01:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1737493004/#ps320001 (title:
"Changing all timestamp back to 32 bits")

[commit-bot: I haz the power, 2016-03-03 01:07:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737493004/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737493004/320001

[commit-bot: I haz the power, 2016-03-03 01:16:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-03 01:16:07]

Try jobs failed on following builders:
  win_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)
  win_drmemory_light on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12896)

[honghaiz3, 2016-03-03 01:23:37]

Patchset #6 (id:320001) has been deleted

[honghaiz3, 2016-03-03 01:24:01]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-03 01:24:02]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1737493004/#ps340001 (title:
"Change back to uint32_t timestamp")

[commit-bot: I haz the power, 2016-03-03 01:24:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737493004/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737493004/340001

[commit-bot: I haz the power, 2016-03-03 03:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-03 03:24:59]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[juberti2, 2016-03-03 05:41:52]

lgtm

[honghaiz3, 2016-03-03 16:26:04]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-03-03 16:26:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737493004/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737493004/340001

[commit-bot: I haz the power, 2016-03-03 16:27:50]

Description was changed from

==========
Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections. 

BUG=
==========

to

==========
Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections. 

BUG=
==========

[commit-bot: I haz the power, 2016-03-03 16:27:51]

Committed patchset #6 (id:340001)

[commit-bot: I haz the power, 2016-03-03 16:28:02]

Description was changed from

==========
Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections. 

BUG=
==========

to

==========
Keep on sending stun binding requests on zero-cost networks.
This is useful to keep the NAT binding alive on backup connections.

BUG=

Committed: https://crrev.com/e2af9ef638408d8ec0a9d0649f8ea86384fa4816
Cr-Commit-Position: refs/heads/master@{#11862}
==========

[commit-bot: I haz the power, 2016-03-03 16:28:03]

Patchset 6 (id:??) landed as
https://crrev.com/e2af9ef638408d8ec0a9d0649f8ea86384fa4816
Cr-Commit-Position: refs/heads/master@{#11862}