From 51b93a541709aa3314baaa1bc751e8daa89c1b7b Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 20 Oct 2023 13:03:16 +0200 Subject: [PATCH] dcsctp: Simplify interface for unchanged timeout When a timer expires, it can optionally return a new expiration value. Clearly, that value can't be zero, as that would make it expire immediately again. To simplify the interface, and make it easier to migrate to rtc::TimeDelta, change it from an optional value to an always-present value that - if zero - means that the expiration time should be unchanged. This is just an internal refactoring, and not part of any external interface. Bug: webrtc:15593 Change-Id: I6e7010d2dbe774ccb260e14fd6b9861c319e2411 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325281 Commit-Queue: Victor Boivie Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#41045} --- net/dcsctp/rx/data_tracker_test.cc | 2 +- net/dcsctp/socket/dcsctp_socket.cc | 12 ++++++------ net/dcsctp/socket/dcsctp_socket.h | 6 +++--- net/dcsctp/socket/heartbeat_handler.cc | 8 ++++---- net/dcsctp/socket/heartbeat_handler.h | 4 ++-- net/dcsctp/socket/stream_reset_handler.cc | 4 ++-- net/dcsctp/socket/stream_reset_handler.h | 2 +- net/dcsctp/socket/stream_reset_handler_test.cc | 4 ++-- net/dcsctp/socket/transmission_control_block.cc | 8 ++++---- net/dcsctp/socket/transmission_control_block.h | 4 ++-- net/dcsctp/timer/timer.cc | 7 ++++--- net/dcsctp/timer/timer.h | 9 +++++---- net/dcsctp/timer/timer_test.cc | 14 +++++++------- net/dcsctp/tx/retransmission_queue_test.cc | 2 +- 14 files changed, 44 insertions(+), 42 deletions(-) diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index 07192fda54..2abc444840 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -42,7 +42,7 @@ class DataTrackerTest : public testing::Test { }), timer_(timer_manager_.CreateTimer( "test/delayed_ack", - []() { return absl::nullopt; }, + []() { return DurationMs(0); }, TimerOptions(DurationMs(0)))), tracker_( std::make_unique("log: ", timer_.get(), kInitialTSN)) { diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index 32bcdaaacf..e47d0554a6 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -921,7 +921,7 @@ bool DcSctpSocket::HandleUnrecognizedChunk( return continue_processing; } -absl::optional DcSctpSocket::OnInitTimerExpiry() { +DurationMs DcSctpSocket::OnInitTimerExpiry() { RTC_DLOG(LS_VERBOSE) << log_prefix() << "Timer " << t1_init_->name() << " has expired: " << t1_init_->expiration_count() << "/" << t1_init_->options().max_restarts.value_or(-1); @@ -933,10 +933,10 @@ absl::optional DcSctpSocket::OnInitTimerExpiry() { InternalClose(ErrorKind::kTooManyRetries, "No INIT_ACK received"); } RTC_DCHECK(IsConsistent()); - return absl::nullopt; + return DurationMs(0); } -absl::optional DcSctpSocket::OnCookieTimerExpiry() { +DurationMs DcSctpSocket::OnCookieTimerExpiry() { // https://tools.ietf.org/html/rfc4960#section-4 // "If the T1-cookie timer expires, the endpoint MUST retransmit COOKIE // ECHO and restart the T1-cookie timer without changing state. This MUST @@ -957,10 +957,10 @@ absl::optional DcSctpSocket::OnCookieTimerExpiry() { } RTC_DCHECK(IsConsistent()); - return absl::nullopt; + return DurationMs(0); } -absl::optional DcSctpSocket::OnShutdownTimerExpiry() { +DurationMs DcSctpSocket::OnShutdownTimerExpiry() { RTC_DLOG(LS_VERBOSE) << log_prefix() << "Timer " << t2_shutdown_->name() << " has expired: " << t2_shutdown_->expiration_count() << "/" @@ -980,7 +980,7 @@ absl::optional DcSctpSocket::OnShutdownTimerExpiry() { InternalClose(ErrorKind::kTooManyRetries, "No SHUTDOWN_ACK received"); RTC_DCHECK(IsConsistent()); - return absl::nullopt; + return DurationMs(0); } // https://tools.ietf.org/html/rfc4960#section-9.2 diff --git a/net/dcsctp/socket/dcsctp_socket.h b/net/dcsctp/socket/dcsctp_socket.h index f91eb3ead4..2240ff629a 100644 --- a/net/dcsctp/socket/dcsctp_socket.h +++ b/net/dcsctp/socket/dcsctp_socket.h @@ -155,9 +155,9 @@ class DcSctpSocket : public DcSctpSocketInterface { // Closes the association, because of too many retransmission errors. void CloseConnectionBecauseOfTooManyTransmissionErrors(); // Timer expiration handlers - absl::optional OnInitTimerExpiry(); - absl::optional OnCookieTimerExpiry(); - absl::optional OnShutdownTimerExpiry(); + DurationMs OnInitTimerExpiry(); + DurationMs OnCookieTimerExpiry(); + DurationMs OnShutdownTimerExpiry(); void OnSentPacket(rtc::ArrayView packet, SendPacketStatus status); // Sends SHUTDOWN or SHUTDOWN-ACK if the socket is shutting down and if all diff --git a/net/dcsctp/socket/heartbeat_handler.cc b/net/dcsctp/socket/heartbeat_handler.cc index 902dff962f..8d1d04a07e 100644 --- a/net/dcsctp/socket/heartbeat_handler.cc +++ b/net/dcsctp/socket/heartbeat_handler.cc @@ -164,7 +164,7 @@ void HeartbeatHandler::HandleHeartbeatAck(HeartbeatAckChunk chunk) { ctx_->ClearTxErrorCounter(); } -absl::optional HeartbeatHandler::OnIntervalTimerExpiry() { +DurationMs HeartbeatHandler::OnIntervalTimerExpiry() { if (ctx_->is_connection_established()) { HeartbeatInfo info(ctx_->callbacks().TimeMillis()); timeout_timer_->set_duration(ctx_->current_rto()); @@ -183,14 +183,14 @@ absl::optional HeartbeatHandler::OnIntervalTimerExpiry() { << log_prefix_ << "Will not send HEARTBEAT when connection not established"; } - return absl::nullopt; + return DurationMs(0); } -absl::optional HeartbeatHandler::OnTimeoutTimerExpiry() { +DurationMs HeartbeatHandler::OnTimeoutTimerExpiry() { // Note that the timeout timer is not restarted. It will be started again when // the interval timer expires. RTC_DCHECK(!timeout_timer_->is_running()); ctx_->IncrementTxErrorCounter("HEARTBEAT timeout"); - return absl::nullopt; + return DurationMs(0); } } // namespace dcsctp diff --git a/net/dcsctp/socket/heartbeat_handler.h b/net/dcsctp/socket/heartbeat_handler.h index 318b02955b..d232a49b87 100644 --- a/net/dcsctp/socket/heartbeat_handler.h +++ b/net/dcsctp/socket/heartbeat_handler.h @@ -50,8 +50,8 @@ class HeartbeatHandler { void HandleHeartbeatAck(HeartbeatAckChunk chunk); private: - absl::optional OnIntervalTimerExpiry(); - absl::optional OnTimeoutTimerExpiry(); + DurationMs OnIntervalTimerExpiry(); + DurationMs OnTimeoutTimerExpiry(); const absl::string_view log_prefix_; Context* ctx_; diff --git a/net/dcsctp/socket/stream_reset_handler.cc b/net/dcsctp/socket/stream_reset_handler.cc index 2094309afe..ac3f95b5dc 100644 --- a/net/dcsctp/socket/stream_reset_handler.cc +++ b/net/dcsctp/socket/stream_reset_handler.cc @@ -347,13 +347,13 @@ void StreamResetHandler::ResetStreams( } } -absl::optional StreamResetHandler::OnReconfigTimerExpiry() { +DurationMs StreamResetHandler::OnReconfigTimerExpiry() { if (current_request_->has_been_sent()) { // There is an outstanding request, which timed out while waiting for a // response. if (!ctx_->IncrementTxErrorCounter("RECONFIG timeout")) { // Timed out. The connection will close after processing the timers. - return absl::nullopt; + return DurationMs(0); } } else { // There is no outstanding request, but there is a prepared one. This means diff --git a/net/dcsctp/socket/stream_reset_handler.h b/net/dcsctp/socket/stream_reset_handler.h index c335130175..2f604f3bf2 100644 --- a/net/dcsctp/socket/stream_reset_handler.h +++ b/net/dcsctp/socket/stream_reset_handler.h @@ -211,7 +211,7 @@ class StreamResetHandler { void HandleResponse(const ParameterDescriptor& descriptor); // Expiration handler for the Reconfig timer. - absl::optional OnReconfigTimerExpiry(); + DurationMs OnReconfigTimerExpiry(); const absl::string_view log_prefix_; Context* ctx_; diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc index 091d717f8a..4b129700ad 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -97,11 +97,11 @@ class StreamResetHandlerTest : public testing::Test { }), delayed_ack_timer_(timer_manager_.CreateTimer( "test/delayed_ack", - []() { return absl::nullopt; }, + []() { return DurationMs(0); }, TimerOptions(DurationMs(0)))), t3_rtx_timer_(timer_manager_.CreateTimer( "test/t3_rtx", - []() { return absl::nullopt; }, + []() { return DurationMs(0); }, TimerOptions(DurationMs(0)))), data_tracker_(std::make_unique("log: ", delayed_ack_timer_.get(), diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index 0621b48e80..c403b064de 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -125,7 +125,7 @@ void TransmissionControlBlock::ObserveRTT(DurationMs rtt) { delayed_ack_timer_->set_duration(delayed_ack_tmo); } -absl::optional TransmissionControlBlock::OnRtxTimerExpiry() { +DurationMs TransmissionControlBlock::OnRtxTimerExpiry() { TimeMs now = callbacks_.TimeMillis(); RTC_DLOG(LS_INFO) << log_prefix_ << "Timer " << t3_rtx_->name() << " has expired"; @@ -139,13 +139,13 @@ absl::optional TransmissionControlBlock::OnRtxTimerExpiry() { SendBufferedPackets(now); } } - return absl::nullopt; + return DurationMs(0); } -absl::optional TransmissionControlBlock::OnDelayedAckTimerExpiry() { +DurationMs TransmissionControlBlock::OnDelayedAckTimerExpiry() { data_tracker_.HandleDelayedAckTimerExpiry(); MaybeSendSack(); - return absl::nullopt; + return DurationMs(0); } void TransmissionControlBlock::MaybeSendSack() { diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h index 46a39d5a7b..240454a684 100644 --- a/net/dcsctp/socket/transmission_control_block.h +++ b/net/dcsctp/socket/transmission_control_block.h @@ -149,9 +149,9 @@ class TransmissionControlBlock : public Context { private: // Will be called when the retransmission timer (t3-rtx) expires. - absl::optional OnRtxTimerExpiry(); + DurationMs OnRtxTimerExpiry(); // Will be called when the delayed ack timer expires. - absl::optional OnDelayedAckTimerExpiry(); + DurationMs OnDelayedAckTimerExpiry(); const absl::string_view log_prefix_; const DcSctpOptions options_; diff --git a/net/dcsctp/timer/timer.cc b/net/dcsctp/timer/timer.cc index 208f26fdf9..1c23d6d270 100644 --- a/net/dcsctp/timer/timer.cc +++ b/net/dcsctp/timer/timer.cc @@ -109,9 +109,10 @@ void Timer::Trigger(TimerGeneration generation) { timeout_->Start(duration, MakeTimeoutId(id_, generation_)); } - absl::optional new_duration = on_expired_(); - if (new_duration.has_value() && new_duration != duration_) { - duration_ = new_duration.value(); + DurationMs new_duration = on_expired_(); + RTC_DCHECK(new_duration != DurationMs::InfiniteDuration()); + if (new_duration > DurationMs(0) && new_duration != duration_) { + duration_ = new_duration; if (is_running_) { // Restart it with new duration. timeout_->Stop(); diff --git a/net/dcsctp/timer/timer.h b/net/dcsctp/timer/timer.h index 95aae570c8..6159737ea0 100644 --- a/net/dcsctp/timer/timer.h +++ b/net/dcsctp/timer/timer.h @@ -102,10 +102,11 @@ class Timer { // The maximum timer duration - one day. static constexpr DurationMs kMaxTimerDuration = DurationMs(24 * 3600 * 1000); - // When expired, the timer handler can optionally return a new duration which - // will be set as `duration` and used as base duration when the timer is - // restarted and as input to the backoff algorithm. - using OnExpired = std::function()>; + // When expired, the timer handler can optionally return a new non-zero + // duration which will be set as `duration` and used as base duration when the + // timer is restarted and as input to the backoff algorithm. If zero is + // returned, the current duration is used. + using OnExpired = std::function; // TimerManager will have pointers to these instances, so they must not move. Timer(const Timer&) = delete; diff --git a/net/dcsctp/timer/timer_test.cc b/net/dcsctp/timer/timer_test.cc index 93876160bb..e0e210e50d 100644 --- a/net/dcsctp/timer/timer_test.cc +++ b/net/dcsctp/timer/timer_test.cc @@ -29,7 +29,7 @@ class TimerTest : public testing::Test { manager_([this](webrtc::TaskQueueBase::DelayPrecision precision) { return timeout_manager_.CreateTimeout(precision); }) { - ON_CALL(on_expired_, Call).WillByDefault(Return(absl::nullopt)); + ON_CALL(on_expired_, Call).WillByDefault(Return(DurationMs(0))); } void AdvanceTimeAndRunTimers(DurationMs duration) { @@ -48,7 +48,7 @@ class TimerTest : public testing::Test { TimeMs now_ = TimeMs(0); FakeTimeoutManager timeout_manager_; TimerManager manager_; - testing::MockFunction()> on_expired_; + testing::MockFunction on_expired_; }; TEST_F(TimerTest, TimerIsInitiallyStopped) { @@ -366,7 +366,7 @@ TEST_F(TimerTest, TimerCanBeStartedFromWithinExpirationHandler) { EXPECT_TRUE(t1->is_running()); t1->set_duration(DurationMs(5000)); t1->Start(); - return absl::nullopt; + return DurationMs(0); }); AdvanceTimeAndRunTimers(DurationMs(1000)); @@ -378,7 +378,7 @@ TEST_F(TimerTest, TimerCanBeStartedFromWithinExpirationHandler) { EXPECT_TRUE(t1->is_running()); t1->set_duration(DurationMs(5000)); t1->Start(); - return absl::make_optional(DurationMs(8000)); + return DurationMs(8000); }); AdvanceTimeAndRunTimers(DurationMs(1)); @@ -435,12 +435,12 @@ TEST(TimerManagerTest, TimerManagerPassesPrecisionToCreateTimeoutMethod) { }); // Default TimerOptions. manager.CreateTimer( - "test_timer", []() { return absl::optional(); }, + "test_timer", []() { return DurationMs(0); }, TimerOptions(DurationMs(123))); EXPECT_EQ(create_timer_precison, webrtc::TaskQueueBase::DelayPrecision::kLow); // High precision TimerOptions. manager.CreateTimer( - "test_timer", []() { return absl::optional(); }, + "test_timer", []() { return DurationMs(0); }, TimerOptions(DurationMs(123), TimerBackoffAlgorithm::kExponential, absl::nullopt, DurationMs::InfiniteDuration(), webrtc::TaskQueueBase::DelayPrecision::kHigh)); @@ -448,7 +448,7 @@ TEST(TimerManagerTest, TimerManagerPassesPrecisionToCreateTimeoutMethod) { webrtc::TaskQueueBase::DelayPrecision::kHigh); // Low precision TimerOptions. manager.CreateTimer( - "test_timer", []() { return absl::optional(); }, + "test_timer", []() { return DurationMs(0); }, TimerOptions(DurationMs(123), TimerBackoffAlgorithm::kExponential, absl::nullopt, DurationMs::InfiniteDuration(), webrtc::TaskQueueBase::DelayPrecision::kLow)); diff --git a/net/dcsctp/tx/retransmission_queue_test.cc b/net/dcsctp/tx/retransmission_queue_test.cc index d50494f084..096bb1262a 100644 --- a/net/dcsctp/tx/retransmission_queue_test.cc +++ b/net/dcsctp/tx/retransmission_queue_test.cc @@ -74,7 +74,7 @@ class RetransmissionQueueTest : public testing::Test { }), timer_(timer_manager_.CreateTimer( "test/t3_rtx", - []() { return absl::nullopt; }, + []() { return DurationMs(0); }, TimerOptions(options_.rto_initial))) {} std::function CreateChunk(