Change initial DTLS retransmission timer from 1 second to 50ms.
This will help ensure a timely DTLS handshake when there's packet
loss. It will likely result in spurious retransmissions (since the
RTT is usually > 50ms), but since exponential backoff is still used,
there will at most be ~4 extra retransmissions. For a time-sensitive
application like WebRTC this seems like a reasonable tradeoff.
R=pthatcher@webrtc.org, juberti@chromium.org, juberti@webrtc.org
Review URL: https://codereview.webrtc.org/1981463002 .
Committed: https://crrev.com/1e435628366fb9fed71632369f05928ed857d8ef
Cr-Original-Commit-Position: refs/heads/master@{#12853}
Cr-Commit-Position: refs/heads/master@{#13159}
diff --git a/webrtc/base/gunit.h b/webrtc/base/gunit.h
index 5ba0bd9..ad52a43 100644
--- a/webrtc/base/gunit.h
+++ b/webrtc/base/gunit.h
@@ -21,25 +21,25 @@
#endif
// Wait until "ex" is true, or "timeout" expires.
-#define WAIT(ex, timeout) \
- for (int64_t start = rtc::TimeMillis(); \
- !(ex) && rtc::TimeMillis() < start + timeout;) { \
- rtc::Thread::Current()->ProcessMessages(0); \
- rtc::Thread::Current()->SleepMs(1); \
+#define WAIT(ex, timeout) \
+ for (int64_t start = rtc::SystemTimeMillis(); \
+ !(ex) && rtc::SystemTimeMillis() < start + timeout;) { \
+ rtc::Thread::Current()->ProcessMessages(0); \
+ rtc::Thread::Current()->SleepMs(1); \
}
// This returns the result of the test in res, so that we don't re-evaluate
// the expression in the XXXX_WAIT macros below, since that causes problems
// when the expression is only true the first time you check it.
-#define WAIT_(ex, timeout, res) \
- do { \
- int64_t start = rtc::TimeMillis(); \
- res = (ex); \
- while (!res && rtc::TimeMillis() < start + timeout) { \
- rtc::Thread::Current()->ProcessMessages(0); \
- rtc::Thread::Current()->SleepMs(1); \
- res = (ex); \
- } \
+#define WAIT_(ex, timeout, res) \
+ do { \
+ int64_t start = rtc::SystemTimeMillis(); \
+ res = (ex); \
+ while (!res && rtc::SystemTimeMillis() < start + timeout) { \
+ rtc::Thread::Current()->ProcessMessages(0); \
+ rtc::Thread::Current()->SleepMs(1); \
+ res = (ex); \
+ } \
} while (0)
// The typical EXPECT_XXXX and ASSERT_XXXXs, but done until true or a timeout.
diff --git a/webrtc/base/logging.cc b/webrtc/base/logging.cc
index 6060362..c951e15 100644
--- a/webrtc/base/logging.cc
+++ b/webrtc/base/logging.cc
@@ -124,7 +124,9 @@
const char* module)
: severity_(sev), tag_(kLibjingle) {
if (timestamp_) {
- int64_t time = TimeSince(LogStartTime());
+ // Use SystemTimeMillis so that even if tests use fake clocks, the timestamp
+ // in log messages represents the real system time.
+ int64_t time = TimeDiff(SystemTimeMillis(), LogStartTime());
// Also ensure WallClockStartTime is initialized, so that it matches
// LogStartTime.
WallClockStartTime();
@@ -210,7 +212,7 @@
}
int64_t LogMessage::LogStartTime() {
- static const int64_t g_start = TimeMillis();
+ static const int64_t g_start = SystemTimeMillis();
return g_start;
}
diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc
index 1d2d03a..fa558fa 100644
--- a/webrtc/base/opensslstreamadapter.cc
+++ b/webrtc/base/opensslstreamadapter.cc
@@ -34,6 +34,7 @@
#include "webrtc/base/openssldigest.h"
#include "webrtc/base/opensslidentity.h"
#include "webrtc/base/stringutils.h"
+#include "webrtc/base/timeutils.h"
#include "webrtc/base/thread.h"
namespace rtc {
@@ -58,7 +59,13 @@
{nullptr, 0}};
#endif
-#ifndef OPENSSL_IS_BORINGSSL
+#ifdef OPENSSL_IS_BORINGSSL
+static void TimeCallback(const SSL* ssl, struct timeval* out_clock) {
+ uint64_t time = TimeNanos();
+ out_clock->tv_sec = time / kNumNanosecsPerSec;
+ out_clock->tv_usec = time / kNumNanosecsPerMicrosec;
+}
+#else // #ifdef OPENSSL_IS_BORINGSSL
// Cipher name table. Maps internal OpenSSL cipher ids to the RFC name.
struct SslCipherMapEntry {
@@ -771,13 +778,19 @@
SSL_set_app_data(ssl_, this);
SSL_set_bio(ssl_, bio, bio); // the SSL object owns the bio now.
-#ifndef OPENSSL_IS_BORINGSSL
if (ssl_mode_ == SSL_MODE_DTLS) {
+#ifdef OPENSSL_IS_BORINGSSL
+ // Change the initial retransmission timer from 1 second to 50ms.
+ // This will likely result in some spurious retransmissions, but
+ // it's useful for ensuring a timely handshake when there's packet
+ // loss.
+ DTLSv1_set_initial_timeout_duration(ssl_, 50);
+#else
// Enable read-ahead for DTLS so whole packets are read from internal BIO
// before parsing. This is done internally by BoringSSL for DTLS.
SSL_set_read_ahead(ssl_, 1);
- }
#endif
+ }
SSL_set_mode(ssl_, SSL_MODE_ENABLE_PARTIAL_WRITE |
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
@@ -985,6 +998,11 @@
DTLS1_2_VERSION : TLS1_2_VERSION);
break;
}
+ // Set a time callback for BoringSSL because:
+ // 1. Our time function is more accurate (doesn't just use gettimeofday).
+ // 2. This allows us to inject a fake clock for testing.
+ // SSL_CTX_set_current_time_cb(ctx, &TimeCallback);
+ ctx->current_time_cb = &TimeCallback;
#endif
if (identity_ && !identity_->ConfigureIdentity(ctx)) {
@@ -1127,6 +1145,14 @@
#endif
}
+bool OpenSSLStreamAdapter::IsBoringSsl() {
+#ifdef OPENSSL_IS_BORINGSSL
+ return true;
+#else
+ return false;
+#endif
+}
+
#define CDEF(X) \
{ static_cast<uint16_t>(TLS1_CK_##X & 0xffff), "TLS_" #X }
diff --git a/webrtc/base/opensslstreamadapter.h b/webrtc/base/opensslstreamadapter.h
index 1e90bac..05e8102 100644
--- a/webrtc/base/opensslstreamadapter.h
+++ b/webrtc/base/opensslstreamadapter.h
@@ -111,6 +111,7 @@
static bool HaveDtls();
static bool HaveDtlsSrtp();
static bool HaveExporter();
+ static bool IsBoringSsl();
static bool IsAcceptableCipher(int cipher, KeyType key_type);
static bool IsAcceptableCipher(const std::string& cipher, KeyType key_type);
diff --git a/webrtc/base/sslstreamadapter.cc b/webrtc/base/sslstreamadapter.cc
index e0fce3a..44158d4 100644
--- a/webrtc/base/sslstreamadapter.cc
+++ b/webrtc/base/sslstreamadapter.cc
@@ -82,6 +82,9 @@
bool SSLStreamAdapter::HaveExporter() {
return OpenSSLStreamAdapter::HaveExporter();
}
+bool SSLStreamAdapter::IsBoringSsl() {
+ return OpenSSLStreamAdapter::IsBoringSsl();
+}
bool SSLStreamAdapter::IsAcceptableCipher(int cipher, KeyType key_type) {
return OpenSSLStreamAdapter::IsAcceptableCipher(cipher, key_type);
}
diff --git a/webrtc/base/sslstreamadapter.h b/webrtc/base/sslstreamadapter.h
index c5045f1..ba60ce3 100644
--- a/webrtc/base/sslstreamadapter.h
+++ b/webrtc/base/sslstreamadapter.h
@@ -195,6 +195,7 @@
static bool HaveDtls();
static bool HaveDtlsSrtp();
static bool HaveExporter();
+ static bool IsBoringSsl();
// Returns true iff the supplied cipher is deemed to be strong.
// TODO(torbjorng): Consider removing the KeyType argument.
diff --git a/webrtc/base/timeutils.cc b/webrtc/base/timeutils.cc
index 3c89d80..1be368e 100644
--- a/webrtc/base/timeutils.cc
+++ b/webrtc/base/timeutils.cc
@@ -38,10 +38,7 @@
return prev;
}
-uint64_t TimeNanos() {
- if (g_clock) {
- return g_clock->TimeNanos();
- }
+uint64_t SystemTimeNanos() {
int64_t ticks;
#if defined(WEBRTC_MAC)
static mach_timebase_info_data_t timebase;
@@ -86,6 +83,17 @@
return ticks;
}
+int64_t SystemTimeMillis() {
+ return static_cast<int64_t>(SystemTimeNanos() / kNumNanosecsPerMillisec);
+}
+
+uint64_t TimeNanos() {
+ if (g_clock) {
+ return g_clock->TimeNanos();
+ }
+ return SystemTimeNanos();
+}
+
uint32_t Time32() {
return static_cast<uint32_t>(TimeNanos() / kNumNanosecsPerMillisec);
}
diff --git a/webrtc/base/timeutils.h b/webrtc/base/timeutils.h
index 113793a..25ef521 100644
--- a/webrtc/base/timeutils.h
+++ b/webrtc/base/timeutils.h
@@ -53,6 +53,11 @@
// that uses it, eliminating the need for a global variable and this function.
ClockInterface* SetClockForTesting(ClockInterface* clock);
+// Returns the actual system time, even if a clock is set for testing.
+// Useful for timeouts while using a test clock, or for logging.
+uint64_t SystemTimeNanos();
+int64_t SystemTimeMillis();
+
// Returns the current time in milliseconds in 32 bits.
uint32_t Time32();
diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
index 486b51a..705df2d 100644
--- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc
+++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
@@ -22,10 +22,10 @@
#include "webrtc/base/sslstreamadapter.h"
#include "webrtc/base/stringutils.h"
-#define MAYBE_SKIP_TEST(feature) \
- if (!(rtc::SSLStreamAdapter::feature())) { \
- LOG(LS_INFO) << "Feature disabled... skipping"; \
- return; \
+#define MAYBE_SKIP_TEST(feature) \
+ if (!(rtc::SSLStreamAdapter::feature())) { \
+ LOG(LS_INFO) << #feature " feature disabled... skipping"; \
+ return; \
}
static const char kIceUfrag1[] = "TESTICEUFRAG0001";
@@ -413,7 +413,8 @@
rtc::SentPacket sent_packet_;
};
-
+// Note that this test always uses a FakeClock, due to the |fake_clock_| member
+// variable.
class DtlsTransportChannelTest : public testing::Test {
public:
DtlsTransportChannelTest()
@@ -561,6 +562,7 @@
}
protected:
+ rtc::ScopedFakeClock fake_clock_;
DtlsTestClient client1_;
DtlsTestClient client2_;
int channel_ct_;
@@ -974,3 +976,45 @@
kTimeout);
EXPECT_EQ(1, client1_.received_dtls_client_hellos());
}
+
+// Test that packets are retransmitted according to the expected schedule.
+// Each time a timeout occurs, the retransmission timer should be doubled up to
+// 60 seconds. The timer defaults to 1 second, but for WebRTC we should be
+// initializing it to 50ms.
+TEST_F(DtlsTransportChannelTest, TestRetransmissionSchedule) {
+ MAYBE_SKIP_TEST(HaveDtls);
+ // We can only change the retransmission schedule with a recently-added
+ // BoringSSL API. Skip the test if not built with BoringSSL.
+ MAYBE_SKIP_TEST(IsBoringSsl);
+
+ PrepareDtls(true, true, rtc::KT_DEFAULT);
+ // Exchange transport descriptions.
+ Negotiate(cricket::CONNECTIONROLE_ACTPASS, cricket::CONNECTIONROLE_ACTIVE);
+
+ // Make client2_ writable, but not client1_.
+ // This means client1_ will send DTLS client hellos but get no response.
+ EXPECT_TRUE(client2_.Connect(&client1_, true));
+ EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
+
+ // Wait for the first client hello to be sent.
+ EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
+ EXPECT_FALSE(client1_.all_raw_channels_writable());
+
+ static int timeout_schedule_ms[] = {50, 100, 200, 400, 800, 1600,
+ 3200, 6400, 12800, 25600, 51200, 60000};
+
+ int expected_hellos = 1;
+ for (size_t i = 0;
+ i < (sizeof(timeout_schedule_ms) / sizeof(timeout_schedule_ms[0]));
+ ++i) {
+ // For each expected retransmission time, advance the fake clock a
+ // millisecond before the expected time and verify that no unexpected
+ // retransmissions were sent. Then advance it the final millisecond and
+ // verify that the expected retransmission was sent.
+ fake_clock_.AdvanceTime(
+ rtc::TimeDelta::FromMilliseconds(timeout_schedule_ms[i] - 1));
+ EXPECT_EQ(expected_hellos, client1_.received_dtls_client_hellos());
+ fake_clock_.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1));
+ EXPECT_EQ(++expected_hellos, client1_.received_dtls_client_hellos());
+ }
+}
