grid: Add a new class for tracking HTTP/3 status #16067
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| #include "common/http/http3_status_tracker.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Http { | ||
|
|
||
| namespace { | ||
|
|
||
| // Initially, HTTP/3 is be marked broken for 5 minutes. | ||
| const std::chrono::minutes DefaultExpirationTime{5}; | ||
RyanTheOptimist marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Cap the broken period at just under 1 day. | ||
| const int MaxConsecutiveBrokenCount = 8; | ||
| } // namespace | ||
|
|
||
| Http3StatusTracker::Http3StatusTracker(Event::Dispatcher& dispatcher) | ||
| : expiration_timer_(dispatcher.createTimer([this]() -> void { onExpirationTimeout(); })) {} | ||
|
|
||
| bool Http3StatusTracker::isHttp3Broken() const { return state_ == State::Broken; } | ||
|
|
||
| bool Http3StatusTracker::isHttp3Confirmed() const { return state_ == State::Confirmed; } | ||
|
|
||
| void Http3StatusTracker::markHttp3Broken() { | ||
| state_ = State::Broken; | ||
|
Contributor
There was a problem hiding this comment. What are valid values for state_ when entering this method?
Contributor
Author
There was a problem hiding this comment. Good question! Mentally, I'm modeling this off of the similar code in Chrome. That code runs up at the request/response layer (the HttpNetworkTransaction) which is above the connection establishment layer. As such and given that requests happen in parallel, it's possible for basically any sequence of markBroken/markConfirmed calls to arrive in any order. I suspect that we'll eventually want something similar. But since we're not doing anything like that now, there's no need to permit such state transitions. So I've added ASSERT() calls to make it clear what the valid states are. Thanks for pointing this out. (In any case, this should be reachable from any state other than broken)
Contributor
There was a problem hiding this comment. It may be possible to trigger this ASSERT, if there are 2 concurrent attempts to connect to the same endpoint. That said, it's possible that there are protections elsewhere to prevent this from happening or it is relatively unlikely to happen without a burst of requests for that service; we would need the number of requests to exceed the upstream's multiplexing factor and trigger creation of a second connection in order to meet demand.
Contributor
Author
There was a problem hiding this comment. Ah! Excellent point! That's very true. Ok, in that case we're back to the Chrome situation where the parallelism means that we can really get any sequence of events in any order. I've removed the ASSERT() calls.
Contributor
There was a problem hiding this comment. Just for my own curiosity why would we have multiple concurrent connection attempts to the same endpoint on a given worker thread? Is this related to prefetching or part of the grid logic?
Contributor
Author
There was a problem hiding this comment. I'm relatively unclear at this on exactly how this all plays together. From what Antonio said, it sound like if there were a sufficient number of simultaneous requests we might trigger the creation of a second attempt. The other case that I think I heard from alyssa is that it's possible to have multiple calls to ConnectivityGrid::newStream() happen before the first call finishes. This won't result in multiple TCP/QUIC connection attempts because the underlying connection pool will do the right thing. But I think this is transparent to the ConnectivtyGrid; each call to newStream creates a new WrapperCallbacks and (up to) 2 ConnectionAttempts which should mean it's possible to get unexpected state transitions. Happy to do something different if I'm not understanding.
Contributor
There was a problem hiding this comment. Yeah this sounds plausible to me: newStream is non-blocking (like most things in Envoy), so if multiple streams are established before the connection is established so you'd see multiple newStreams come in before the connection is established. No change necessary, I was just curious how this all fit together :)
Contributor
There was a problem hiding this comment. Optional (I mean in this PR, not optional overall) I wonder if we should start landing docs on this as we start implementing the "real" logic. Generally we land docs when we unhide config (#15926) but the failover logic is sufficiently complicated I think we could land docs for now in source/docs and then move them to docs/ when the PR lands. Your call if we do them now or in a future iteration :-)
Contributor
Author
There was a problem hiding this comment. Docs definitely make sense and I'm happy to work on them. I think I'll do that in a follow-up since this PR is (hopefully) basically done at this point. |
||
| if (!expiration_timer_->enabled()) { | ||
| expiration_timer_->enableTimer(std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| DefaultExpirationTime * (1 << consecutive_broken_count_))); | ||
| if (consecutive_broken_count_ < MaxConsecutiveBrokenCount) { | ||
| ++consecutive_broken_count_; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void Http3StatusTracker::markHttp3Confirmed() { | ||
| state_ = State::Confirmed; | ||
| consecutive_broken_count_ = 0; | ||
| if (expiration_timer_->enabled()) { | ||
| expiration_timer_->disableTimer(); | ||
| } | ||
| } | ||
|
|
||
| void Http3StatusTracker::onExpirationTimeout() { | ||
| if (state_ != State::Broken) { | ||
| return; | ||
RyanTheOptimist marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| state_ = State::Pending; | ||
| } | ||
|
|
||
| } // namespace Http | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/event/dispatcher.h" | ||
| #include "envoy/event/timer.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Http { | ||
|
|
||
| // Tracks the status of HTTP/3 being broken for a period of time | ||
| // subject to exponential backoff. | ||
| class Http3StatusTracker { | ||
| public: | ||
| explicit Http3StatusTracker(Event::Dispatcher& dispatcher); | ||
|
|
||
| // Returns true if HTTP/3 is broken. | ||
| bool isHttp3Broken() const; | ||
| // Returns true if HTTP/3 is confirmed to be working. | ||
| bool isHttp3Confirmed() const; | ||
RyanTheOptimist marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Marks HTTP/3 broken for a period of time, subject to backoff. | ||
| void markHttp3Broken(); | ||
| // Marks HTTP/3 as confirmed to be working and resets the backoff timeout. | ||
| void markHttp3Confirmed(); | ||
|
|
||
| private: | ||
| enum class State { | ||
| Pending, | ||
| Broken, | ||
| Confirmed, | ||
| }; | ||
|
|
||
| // Called when the expiration timer fires. | ||
| void onExpirationTimeout(); | ||
|
|
||
| State state_{State::Pending}; | ||
| // The number of consecutive times HTTP/3 has been marked broken. | ||
| int consecutive_broken_count_{}; | ||
| // The timer which tracks when HTTP/3 broken status should expire | ||
| Event::TimerPtr expiration_timer_; | ||
| }; | ||
|
|
||
| } // namespace Http | ||
| } // namespace Envoy | ||
Uh oh!
There was an error while loading. Please reload this page.