[chttp2] Fix bug in timeout encoding #34751
Conversation
ctiller
added
the
release notes: no
Automated fix for refs/heads/smol
| }; | ||
|
|
||
| class TimeoutCompressorImpl { | ||
| public: | ||
| void EncodeWith(absl::string_view key, Timestamp deadline, Encoder* encoder); | ||
|
|
||
| private: | ||
| std::vector<PreviousTimeout> previous_timeouts_; | ||
| enum { kNumPreviousValues = 5 }; |
There was a problem hiding this comment.
in C++98 this was the way to say static constexpr const kNumPreviousValues = 5;... and:
- old habits are hard to break :)
- having typed that I'm not clear we've made progress
still, updated to the more modern syntax
There was a problem hiding this comment.
sorry, i am afraid that this problem is important or not ?i am reall sorry to bother
| double ratio = timeout.RatioVersus(it->timeout); | ||
| for (size_t i = 0; i < kNumPreviousValues; i++) { | ||
| const auto& previous = previous_timeouts_[i]; | ||
| if (!table.ConvertableToDynamicIndex(previous.index)) continue; |
There was a problem hiding this comment.
will this if condition return if previous_timeouts_[i] has not been populated yet, like if the index is 0? What does ConvertableToDynamicIndex check for?
There was a problem hiding this comment.
added some comments around the safety of this
| // be considered earlier. | ||
| std::swap(*it, *previous_timeouts_.begin()); | ||
| if (ratio > -3 && ratio <= 0) { | ||
| encoder->EmitIndexed(table.DynamicIndex(previous.index)); |
There was a problem hiding this comment.
Will there be any negative effects to emitting these timeouts directly rather than adding to the queue first? What if there are a lot of timeouts like this?
There was a problem hiding this comment.
If we hit this sufficiently frequently this could cause some significant hpack table churn... evidence so far says that we'll not hit it frequently, but it'll be something to watch.
grpc-checks
bot
added
per-call-memory/decrease
bloat/none
and removed
per-call-memory/neutral
bloat/improvement
labels
grpc-checks
bot
added
per-call-memory/neutral
and removed
per-call-memory/decrease
labels
copybara-service
bot
added
the
imported
Just the right combination of timeout values could lead to a large amount of growth in the timeout cache, leading to high memory usage and high cpu utilization on the client attempting that encoding.
Fix: cap the number of cached timeout values we'll consider to a very small number (I'm choosing five).
Add a fuzzer that ensures that we're actually respecting the limits of inaccuracy we're imposing upon ourselves.