Repository navigation
cherry pick three commits from v13 to v14 - #339
Conversation
Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
Histogram observations above the largest configured boundary use the implicit overflow bucket at index boundaries.size(). RCU and thread-buffer aggregation previously omitted that slot during merge, leaving the sum updated while count and the Prometheus +Inf bucket were too low. The atomic implementation discarded these observations entirely. Account for the implicit overflow bucket in every implementation, validate that configured boundaries fit the fixed histogram storage, and cover below-boundary, exact-boundary, and overflow observations across all backends and Prometheus output.
|
|
||
| private: | ||
| friend class WatchdogRegistry; | ||
| WatchdogRegistry* registry_{nullptr}; |
There was a problem hiding this comment.
Nothing detaches live watchdogs when the registry goes away. If a Watchdog outlives the registry, it accesses nullptrs at shutdown and crashes.
|
|
||
| Watchdog::~Watchdog() { | ||
| if (registry_ == nullptr) { return; } | ||
| registry_->remove(id_); |
There was a problem hiding this comment.
May be a race/use-after-free here. Watchdog only removes itself from the registry in the base destructor, which runs after the derived classes have already been destroyed. When LeaseWatchdog is destroyed, ~DeadlineWatchdog() runs and frees name_. Another thread being in the middle of failures() and calls check(), it's dealing with half-destroyed object, e.g. accessing name_ is use-after-free.
| std::vector< WatchdogFailure > WatchdogRegistry::failures() const { | ||
| std::vector< WatchdogFailure > out; | ||
| auto const now = std::chrono::steady_clock::now(); | ||
| std::lock_guard< std::mutex > lock(mu_); |
There was a problem hiding this comment.
Is it possible to deadlock here if a subclass calls failures() while the lock is held?
|
@nnastonen thanks for the nice finding. I have updated this PR to address your comments. ptal @Besroy can you also take a look at the change of watchdog? |
Besroy
left a comment
Besroy
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the review and fix!
4 participants
this PR cherry pick three commits from v13 to v14, no new logic change