Define a ThrottlingData internal class to manage the throttling logic In order to mitigate the consequences of an aggressive throttling one of the actions proposed in the design doc [1] is to define a map indexed by the server's IP address to store the pending queues. This change defines an internal class that will be eventually stored in such map, so that we can retrieve the pending queue associated to a specific IP. [1]https://docs.google.com/document/d/1j2NJqLQyAHSBZArW6Iwg8uVId3NGQxnBV7b9LYbd8FM/edit?usp=sharing Bug: 40069954 Change-Id: Iaca8a6cb9d0194c066d781f6a6472252cfb8876c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5664914 Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Javier Fernandez <jfernandez@igalia.com> Cr-Commit-Position: refs/heads/main@{#1325370}
diff --git a/content/browser/webtransport/web_transport_throttle_context.cc b/content/browser/webtransport/web_transport_throttle_context.cc index 69ca10eb..75c9607a 100644 --- a/content/browser/webtransport/web_transport_throttle_context.cc +++ b/content/browser/webtransport/web_transport_throttle_context.cc
@@ -24,15 +24,77 @@ } // namespace +WebTransportThrottleContext::PenaltyManager::PenaltyManager( + WebTransportThrottleContext* throttle_context) + : throttle_context_(throttle_context) {} + +WebTransportThrottleContext::PenaltyManager::~PenaltyManager() = default; + +void WebTransportThrottleContext::PenaltyManager::QueuePending( + base::TimeDelta after) { + DVLOG(1) << "WebTransportThrottleContext::QueuePending() this=" << this + << " after=" << after + << " pending_handshakes_= " << pending_handshakes_; + + const auto when = base::TimeTicks::Now() + after; + if (pending_queue_.empty() || when < pending_queue_.top()) { + StartPendingQueueTimer(after); + } + pending_queue_.push(when); +} + +void WebTransportThrottleContext::PenaltyManager::MaybeDecrementPending() { + DVLOG(1) << "WebTransportThrottleContext::MaybeDecrementPending() this=" + << this << " pending_handshakes_= " << pending_handshakes_; + + const auto now = base::TimeTicks::Now(); + while (!pending_queue_.empty() && pending_queue_.top() <= now) { + pending_queue_.pop(); + --pending_handshakes_; + } + throttle_context_->OnPendingQueueReady(); + + ProcessPendingQueue(); +} + +void WebTransportThrottleContext::PenaltyManager::ProcessPendingQueue() { + if (pending_queue_.empty()) { + return; + } + + StartPendingQueueTimer(pending_queue_.top() - base::TimeTicks::Now()); +} + +void WebTransportThrottleContext::PenaltyManager::StopPendingQueueTimer() { + if (pending_queue_timer_.IsRunning()) { + pending_queue_timer_.Stop(); + } +} + +void WebTransportThrottleContext::PenaltyManager::StartPendingQueueTimer( + base::TimeDelta after) { + DVLOG(1) << "WebTransportThrottleContext::StartPendingQueueTimer() this=" + << this << " after=" << after + << " pending_handshakes_= " << pending_handshakes_; + + // This use of base::Unretained is safe because this timer is owned by this + // object and will be stopped on destruction. + pending_queue_timer_.Start( + FROM_HERE, after, + base::BindOnce(&PenaltyManager::MaybeDecrementPending, + base::Unretained(this))); +} + WebTransportThrottleContext::Tracker::Tracker( base::WeakPtr<WebTransportThrottleContext> throttle_context) : throttle_context_(throttle_context) { - DVLOG(1) << "WebTransportThrottleContext::Tracker()" - << " this=" << this << " pending_handshakes_= " - << throttle_context_->pending_handshakes_; + DVLOG(1) << "WebTransportThrottleContext::Tracker()" << " this=" << this + << " pending_handshakes_= " + << throttle_context_->penalty_mgr_.PendingHandshakes(); DCHECK(throttle_context_); - DCHECK_LT(throttle_context_->pending_handshakes_, kMaxPendingSessions); - ++throttle_context_->pending_handshakes_; + DCHECK_LT(throttle_context_->penalty_mgr_.PendingHandshakes(), + kMaxPendingSessions); + throttle_context_->penalty_mgr_.AddPendingHandshakes(); } WebTransportThrottleContext::Tracker::~Tracker() { @@ -49,9 +111,9 @@ return; DVLOG(1) << " pending_handshakes_= " - << throttle_context_->pending_handshakes_; - DCHECK_GT(throttle_context_->pending_handshakes_, 0); - throttle_context_->QueuePending(base::Milliseconds(10)); + << throttle_context_->penalty_mgr_.PendingHandshakes(); + DCHECK_GT(throttle_context_->penalty_mgr_.PendingHandshakes(), 0); + throttle_context_->penalty_mgr_.QueuePending(base::Milliseconds(10)); throttle_context_ = nullptr; } @@ -63,7 +125,7 @@ return; DVLOG(1) << " pending_handshakes_= " - << throttle_context_->pending_handshakes_; + << throttle_context_->penalty_mgr_.PendingHandshakes(); throttle_context_->MaybeQueueHandshakeFailurePenalty(); throttle_context_ = nullptr; } @@ -78,16 +140,17 @@ WebTransportThrottleContext::PerformThrottle( ThrottleDoneCallback on_throttle_done) { DVLOG(1) << "WebTransportThrottleContext::PerformThrottle() this=" << this - << " pending_handshakes_=" << pending_handshakes_; + << " pending_handshakes_=" << penalty_mgr_.PendingHandshakes(); - if (!pending_queue_timer_.IsRunning()) { + if (!penalty_mgr_.PendingQueueTimerIsRunning()) { // If the timer was not running there may be some pending connections that // were not cleaned up yet. May cause other handshakes to be started as a // side-effect, but since they are unrelated this is harmless. - MaybeDecrementPending(); + penalty_mgr_.MaybeDecrementPending(); } - if (pending_handshakes_ + static_cast<int>(throttled_connections_.size()) >= + if (penalty_mgr_.PendingHandshakes() + + static_cast<int>(throttled_connections_.size()) >= kMaxPendingSessions) { return ThrottleResult::kTooManyPendingSessions; } @@ -98,9 +161,9 @@ ScheduleThrottledConnection(); } - if (!pending_queue_timer_.IsRunning() && !pending_queue_.empty() && + if (!penalty_mgr_.PendingQueueTimerIsRunning() && !throttled_connections_.empty()) { - StartPendingQueueTimer(pending_queue_.top() - base::TimeTicks::Now()); + penalty_mgr_.ProcessPendingQueue(); } return ThrottleResult::kOk; @@ -108,11 +171,11 @@ void WebTransportThrottleContext::MaybeQueueHandshakeFailurePenalty() { if (should_queue_handshake_failure_penalty_) { - QueuePending(base::Minutes(5)); + penalty_mgr_.QueuePending(base::Minutes(5)); return; } - CHECK_GE(pending_handshakes_, 0); - --pending_handshakes_; + CHECK_GE(penalty_mgr_.PendingHandshakes(), 0); + penalty_mgr_.RemovePendingHandshakes(); } base::WeakPtr<WebTransportThrottleContext> @@ -120,26 +183,33 @@ return weak_factory_.GetWeakPtr(); } +void WebTransportThrottleContext::OnPendingQueueReady() { + if (!throttled_connections_.empty()) { + ScheduleThrottledConnection(); + } +} + void WebTransportThrottleContext::ScheduleThrottledConnection() { DVLOG(1) << "WebTransportThrottleContext::ScheduleThrottledConnection() this=" - << this << " pending_handshakes_= " << pending_handshakes_; + << this + << " pending_handshakes_= " << penalty_mgr_.PendingHandshakes(); DCHECK(!throttled_connections_.empty()); - if (pending_handshakes_ == 0) { + if (penalty_mgr_.PendingHandshakes() == 0) { DoOnThrottleDone(); return; } - DCHECK_GT(pending_handshakes_, 0); + DCHECK_GT(penalty_mgr_.PendingHandshakes(), 0); // Don't do the calculation for large values of `pending_handshakes_` to avoid // integer overflow. If `pending_handshakes_` is 14, the result of the // calculation is 81920, so it will always get truncated to 60000. const int milliseconds_delay = - pending_handshakes_ > 13 + penalty_mgr_.PendingHandshakes() > 13 ? 60000 - : std::min(10 * (1 << (pending_handshakes_ - 1)), 60000); + : std::min(10 * (1 << (penalty_mgr_.PendingHandshakes() - 1)), 60000); // We multiply the timeout by a random factor so that when a server falls over // and the client code starts to accidentally DoS it, all the clients don't @@ -170,15 +240,15 @@ void WebTransportThrottleContext::DoOnThrottleDone() { DVLOG(1) << "WebTransportThrottleContext::DoOnThrottleDone() this=" << this - << " pending_handshakes_= " << pending_handshakes_ + << " pending_handshakes_= " << penalty_mgr_.PendingHandshakes() << " throttled_connections_.size()=" << throttled_connections_.size(); DCHECK(!throttled_connections_.empty()); auto on_throttle_done = std::move(throttled_connections_.front()); throttled_connections_.pop(); queue_head_time_ = base::TimeTicks::Now(); - if (throttled_connections_.empty() && pending_queue_timer_.IsRunning()) { - pending_queue_timer_.Stop(); + if (throttled_connections_.empty()) { + penalty_mgr_.StopPendingQueueTimer(); } auto tracker = std::make_unique<Tracker>(GetWeakPtr()); std::move(on_throttle_done).Run(std::move(tracker)); @@ -186,7 +256,7 @@ void WebTransportThrottleContext::StartOneConnection() { DVLOG(1) << "WebTransportThrottleContext::StartOneConnection() this=" << this - << " pending_handshakes_= " << pending_handshakes_; + << " pending_handshakes_= " << penalty_mgr_.PendingHandshakes(); if (throttled_connections_.empty()) return; @@ -196,50 +266,4 @@ } } -void WebTransportThrottleContext::QueuePending(base::TimeDelta after) { - DVLOG(1) << "WebTransportThrottleContext::QueuePending() this=" << this - << " after=" << after - << " pending_handshakes_= " << pending_handshakes_; - - const auto when = base::TimeTicks::Now() + after; - if (!throttled_connections_.empty() && - (pending_queue_.empty() || when < pending_queue_.top())) { - StartPendingQueueTimer(after); - } - pending_queue_.push(when); -} - -void WebTransportThrottleContext::MaybeDecrementPending() { - DVLOG(1) << "WebTransportThrottleContext::MaybeDecrementPending() this=" - << this << " pending_handshakes_= " << pending_handshakes_; - - const auto now = base::TimeTicks::Now(); - while (!pending_queue_.empty() && pending_queue_.top() <= now) { - pending_queue_.pop(); - --pending_handshakes_; - } - if (!throttled_connections_.empty()) { - ScheduleThrottledConnection(); - } - - if (!pending_queue_.empty() && !throttled_connections_.empty()) { - StartPendingQueueTimer(pending_queue_.top() - base::TimeTicks::Now()); - } -} - -void WebTransportThrottleContext::StartPendingQueueTimer( - base::TimeDelta after) { - DVLOG(1) << "WebTransportThrottleContext::StartPendingQueueTimer() this=" - << this << " after=" << after - << " pending_handshakes_= " << pending_handshakes_; - - DCHECK(!throttled_connections_.empty()); - // This use of base::Unretained is safe because this timer is owned by this - // object and will be stopped on destruction. - pending_queue_timer_.Start( - FROM_HERE, after, - base::BindOnce(&WebTransportThrottleContext::MaybeDecrementPending, - base::Unretained(this))); -} - } // namespace content
diff --git a/content/browser/webtransport/web_transport_throttle_context.h b/content/browser/webtransport/web_transport_throttle_context.h index bd2e6f5..65192a8d 100644 --- a/content/browser/webtransport/web_transport_throttle_context.h +++ b/content/browser/webtransport/web_transport_throttle_context.h
@@ -80,9 +80,64 @@ // explicitly suppressed. void MaybeQueueHandshakeFailurePenalty(); + void OnPendingQueueReady(); + base::WeakPtr<WebTransportThrottleContext> GetWeakPtr(); private: + class PenaltyManager final { + public: + explicit PenaltyManager(WebTransportThrottleContext*); + PenaltyManager(const PenaltyManager&) = delete; + PenaltyManager& operator=(const PenaltyManager&) = delete; + ~PenaltyManager(); + + // Queues a pending handshake to be considered complete after `after`. + void QueuePending(base::TimeDelta after); + + // If there are handshakes in `pending_queue_` that can now be considered + // finished, remove them and decrement `pending_handshakes_`. Recalculates + // the delay for the head of `throttled_connections_` and may trigger it to + // start as a side-effect. + void MaybeDecrementPending(); + + // Start the timer based on the `pending_queue` top. + void ProcessPendingQueue(); + + void StopPendingQueueTimer(); + + int PendingHandshakes() const { return pending_handshakes_; } + void AddPendingHandshakes() { ++pending_handshakes_; } + void RemovePendingHandshakes() { --pending_handshakes_; } + bool PendingQueueTimerIsRunning() { + return pending_queue_timer_.IsRunning(); + } + bool PendingQueueIsEmpty() { return pending_queue_.empty(); } + + private: + // Start the timer for removing items from `pending_queue_timer_`, to fire + // after `after` has passed. + void StartPendingQueueTimer(base::TimeDelta after); + + const raw_ptr<WebTransportThrottleContext> throttle_context_; + + int pending_handshakes_ = 0; + + // Sessions for which the handshake has completed but we are still counting + // as "pending" for the purposes of throttling. An items is added to this + // queue when the handshake completes, and removed when the timer expires. + // The "top" of the queue is the timer that will expire first. + std::priority_queue<base::TimeTicks, + std::vector<base::TimeTicks>, + std::greater<>> + pending_queue_; + + // A timer that will fire the next time an entry should be removed from + // `pending_queue_`. The timer doesn't run when `throttled_connections_` is + // empty. + base::OneShotTimer pending_queue_timer_; + }; + // Starts a connection immediately if there are none pending, or sets a timer // to start one later. void ScheduleThrottledConnection(); @@ -95,33 +150,11 @@ // the next connection. void StartOneConnection(); - // Queues a pending handshake to be considered complete after `after`. - void QueuePending(base::TimeDelta after); - - // If there are handshakes in `pending_queue_` that can now be considered - // finished, remove them and decrement `pending_handshakes_`. Recalculates the - // delay for the head of `throttled_connections_` and may trigger it to start - // as a side-effect. - void MaybeDecrementPending(); - - // Start the timer for removing items from `pending_queue_timer_`, to fire - // after `after` has passed. - void StartPendingQueueTimer(base::TimeDelta after); - // False when the `--webtransport-developer-mode` flag is specified, true // otherwise. const bool should_queue_handshake_failure_penalty_; - int pending_handshakes_ = 0; - - // Sessions for which the handshake has completed but we are still counting as - // "pending" for the purposes of throttling. An items is added to this queue - // when the handshake completes, and removed when the timer expires. The "top" - // of the queue is the timer that will expire first. - std::priority_queue<base::TimeTicks, - std::vector<base::TimeTicks>, - std::greater<>> - pending_queue_; + PenaltyManager penalty_mgr_{this}; base::queue<ThrottleDoneCallback> throttled_connections_; @@ -129,11 +162,6 @@ // This is needed if it gets recheduled by ScheduleThrottledConnection(). base::TimeTicks queue_head_time_; - // A timer that will fire the next time an entry should be removed from - // `pending_queue_`. The timer doesn't run when `throttled_connections_` is - // empty. - base::OneShotTimer pending_queue_timer_; - // A timer that will fire the next time a throttled connection should be // allowed to proceed. This is a reset when pending_handshakes_ is // decremented.