Pick the DTLS handshake timeout based on the ICE RTT estimate

Pick the DTLS handshake timeout based on the ICE RTT estimate

This CL replaces the previously-hardcoded DTLS timeout value of 50 ms with one that is picked dynamically as 2x ICE RTT (clamped between 50 and 3000 ms to keep it reasonable).

No tests yet - I'm working on these, but sending out this CL early to get feedback. I've tested it manually and on slow networks it results in successful DTLS setup in a single attempt - instead of clogging up the slow link with multiple retransmissions as done previosly.

BUG=webrtc:7099
Review-Url: https://codereview.webrtc.org/2670113002
Cr-Commit-Position: refs/heads/master@{#16426}
Committed: https://chromium.googlesource.com/external/webrtc/+/d030912de4d49e247533892927819c4a7f055815

[skvlad, 2017-02-02 07:17:30]

The CQ bit was checked by skvlad@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 07:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[skvlad, 2017-02-02 07:18:43]

skvlad@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[skvlad, 2017-02-02 07:18:43]

PTAL. This is missing unit tests, but I'm interested in your thoughs on the
overall approach.

[commit-bot: I haz the power, 2017-02-02 07:26:29]

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

[commit-bot: I haz the power, 2017-02-02 07:26:30]

Dry run: Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/9177)

[Taylor Brandstetter, 2017-02-02 18:07:42]

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:527: RTC_DCHECK(ssl_ctx_ == NULL);
Should leave a comment in the header file saying that this can only be called
before StartSSL.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:810: // bandwidth usage.
This comment is a little out of place here; this class doesn't know how the
timer value was selected.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:162: virtual void SetHandshakeTimeout(int
timeout_ms) = 0;
nit: Better name would be "InitialRetransmissionTimeout"

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:705: rtc::Optional<int> rtt =
ice_transport_->GetRttEstimate();
If no round trips have occurred yet (which would happen with the "presume
TURN<->TURN connections will work" option), won't this return the max RTT, and
result in a configured timeout of 3 seconds?

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.h:110: rtc::Optional<int> GetRttEstimate()
override;
This method isn't really necessary, since the DTLS transport can call
"selected_connection()" and get its RTT directly.

But if you do decide to keep it, it should be named "GetSmoothedRtt", to avoid
confusion with "ConservativeRTTEstimate" (which returns the smoothed RTT times
2).

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc#newco...
webrtc/p2p/base/port.cc:1258: rtt_ = rtt;
May make more sense in a separate CL, but it would be great if this was renamed
to "smoothed_rtt_" and changed to an rtc::Optional (as opposed to using 3
seconds as an initial value).

[Taylor Brandstetter, 2017-02-02 18:13:31]

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:705: rtc::Optional<int> rtt =
ice_transport_->GetRttEstimate();
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> If no round trips have occurred yet (which would happen with the "presume
> TURN<->TURN connections will work" option), won't this return the max RTT, and
> result in a configured timeout of 3 seconds?

This is probably why the "EndToEndConnectionTimeWithTurnTurnPair" test is
failing:

"configuring DTLS handshake timeout 3000 based on ICE RTT 3000"

I guess that test ended up being useful after all.

[pthatcher2, 2017-02-02 20:09:43]

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

[pthatcher2, 2017-02-02 20:09:44]

We need to not change the default behavior, because it's too risky.  We should
allow apps to opt into this, which means adding it to RTCConfig.  I think it
might be reasonable to have two settings in RTCConfig:

1.  The min value. 
2.  Indicate whether it should be based on RTT.


We could do #1 in this CL and #2 in a followup CL.  #1 is sufficient for 2G
cases (just set the min to 1000).

Of course, if you think you can get it into one CL, that's fine.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:162: virtual void SetHandshakeTimeout(int
timeout_ms) = 0;
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> nit: Better name would be "InitialRetransmissionTimeout"

+1

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc#newco...
webrtc/p2p/base/port.cc:1258: rtt_ = rtt;
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> May make more sense in a separate CL, but it would be great if this was
renamed
> to "smoothed_rtt_" and changed to an rtc::Optional (as opposed to using 3
> seconds as an initial value).

I agree that we should split it into "smoothed_rtt_ and "last_rtt_".  And I
think the DTLS code should used the last_rtt_, not the smoothed_rtt_.

[pthatcher2, 2017-02-02 21:52:58]

lgtm

Forget my other comments. You guys convinced me this is the right way to go.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.h:110: rtc::Optional<int> GetRttEstimate()
override;
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> This method isn't really necessary, since the DTLS transport can call
> "selected_connection()" and get its RTT directly.
> 
> But if you do decide to keep it, it should be named "GetSmoothedRtt", to avoid
> confusion with "ConservativeRTTEstimate" (which returns the smoothed RTT times
> 2).

I'd prefer that the DltsTransport only look at the IceTransport, and not thing
underneath.  It makes it easier to make changes later by staying decoupled.

[skvlad, 2017-02-02 22:17:57]

The CQ bit was checked by skvlad@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 22:18:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[skvlad, 2017-02-02 22:19:33]

PTAL. I've made changes according to code review feedback.
I had to remove the RTT check in the GetStats() test as it was relying on the
previous logic - weighted average with 3000 ms - and testing if the RTT is
greater than zero. The RTT in that test actually _is_ zero.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:527: RTC_DCHECK(ssl_ctx_ == NULL);
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> Should leave a comment in the header file saying that this can only be called
> before StartSSL.

Done.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:810: // bandwidth usage.
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> This comment is a little out of place here; this class doesn't know how the
> timer value was selected.

Acknowledged.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:162: virtual void SetHandshakeTimeout(int
timeout_ms) = 0;
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> nit: Better name would be "InitialRetransmissionTimeout"
I made it InitialHandshakeRetransmissionTimeout. Long but accurate.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:705: rtc::Optional<int> rtt =
ice_transport_->GetRttEstimate();
On 2017/02/02 18:13:31, Taylor Brandstetter wrote:
> On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> > If no round trips have occurred yet (which would happen with the "presume
> > TURN<->TURN connections will work" option), won't this return the max RTT,
and
> > result in a configured timeout of 3 seconds?
> 
> This is probably why the "EndToEndConnectionTimeWithTurnTurnPair" test is
> failing:
> 
> "configuring DTLS handshake timeout 3000 based on ICE RTT 3000"
> 
> I guess that test ended up being useful after all.

I've changed P2PTransportChannel->GetRttEstimate() to only use the RTT if there
is at least one sample.
Switching RTT to rtc::Optional() everywhere will change the existing behavior
for GetStats and many other places - which might be better done in a separate
CL.

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/p2p/base/port.cc#newco...
webrtc/p2p/base/port.cc:1258: rtt_ = rtt;
On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> May make more sense in a separate CL, but it would be great if this was
renamed
> to "smoothed_rtt_" and changed to an rtc::Optional (as opposed to using 3
> seconds as an initial value).

Let's make it rtc::Optional in a separate CL. There are too many places that
depend on there being some value (3000 or anything) that will break - such as
GetStats() or the connection sorting logic.

[pthatcher2, 2017-02-02 22:48:16]

lgtm

[Taylor Brandstetter, 2017-02-02 22:57:27]

lgtm, though I'd still prefer one last renaming (sorry)

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2670113002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:162: virtual void SetHandshakeTimeout(int
timeout_ms) = 0;
On 2017/02/02 22:19:33, skvlad wrote:
> On 2017/02/02 18:07:41, Taylor Brandstetter wrote:
> > nit: Better name would be "InitialRetransmissionTimeout"
> I made it InitialHandshakeRetransmissionTimeout. Long but accurate.

I'd still call it just "InitialRetransmissionTimeout", since in theory it
applies to any DTLS messages (not just the handshake).

[commit-bot: I haz the power, 2017-02-02 23:08:35]

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

[commit-bot: I haz the power, 2017-02-02 23:08:36]

Dry run: This issue passed the CQ dry run.

[skvlad, 2017-02-03 00:08:56]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2017-02-03 00:08:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2670113002/#ps40001 (title:
"Rename InitialHandshakeRetransmissionTimeout -> InitialRetransmissionTimeout.")

[commit-bot: I haz the power, 2017-02-03 00:09:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-03 01:18:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486080536313600,
"parent_rev": "a24a9e2fa667a99f91c1e1bd2c72fe1260962441", "commit_rev":
"d030912de4d49e247533892927819c4a7f055815"}

[commit-bot: I haz the power, 2017-02-03 01:18:41]

Description was changed from

==========
Pick the DTLS handshake timeout based on the ICE RTT estimate

This CL replaces the previously-hardcoded DTLS timeout value of 50 ms with one
that is picked dynamically as 2x ICE RTT (clamped between 50 and 3000 ms to keep
it reasonable).

No tests yet - I'm working on these, but sending out this CL early to get
feedback. I've tested it manually and on slow networks it results in successful
DTLS setup in a single attempt - instead of clogging up the slow link with
multiple retransmissions as done previosly.

BUG=webrtc:7099
==========

to

==========
Pick the DTLS handshake timeout based on the ICE RTT estimate

This CL replaces the previously-hardcoded DTLS timeout value of 50 ms with one
that is picked dynamically as 2x ICE RTT (clamped between 50 and 3000 ms to keep
it reasonable).

No tests yet - I'm working on these, but sending out this CL early to get
feedback. I've tested it manually and on slow networks it results in successful
DTLS setup in a single attempt - instead of clogging up the slow link with
multiple retransmissions as done previosly.

BUG=webrtc:7099

Review-Url: https://codereview.webrtc.org/2670113002
Cr-Commit-Position: refs/heads/master@{#16426}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d030912de4d49e24753389292...
==========

[commit-bot: I haz the power, 2017-02-03 01:18:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/d030912de4d49e24753389292...

[skvlad, 2017-02-03 01:37:25]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/2671903002/ by skvlad@webrtc.org.

The reason for reverting is: Breaks Chromium FYI bots..

[skvlad, 2017-02-03 01:43:11]

On 2017/02/03 01:37:25, skvlad wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.webrtc.org/2671903002/ by mailto:skvlad@webrtc.org.
> 
> The reason for reverting is: Breaks Chromium FYI bots..

Looks like I can't submit the revert... anyway I created a fix in
https://codereview.webrtc.org/2677613002/.