RLQS: Improve timer lifetime management

[yanavlasov]

1. Refactored GlobalRateLimitClientImpl to inherit from std::enable_shared_from_this.
2. Updated all asynchronous callbacks in GlobalRateLimitClientImpl to capture a weak_ptr to 'this' and use the lock-and-check pattern.
3. Changed TlsStore to hold a std::shared_ptr and implement a custom deleter for std::shared_ptr to ensure main-thread cleanup, even when dropped by worker threads.
4. Updated LocalRateLimitClientImpl to hold a std::shared_ptr, anchoring the lifetime of the global client and its resources for the duration of the filter.
5. Updated unit tests to align with the new lifetime management model.

Risk Level: medium
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

[yanavlasov]

@bsurber for review

[bsurber]

LGTM once you have the last bit of coverage

[adisuissa]

LGTM, thanks!
Assigning @tyxia as code-owner and maintainer review.
/assign @tyxia

[adisuissa]

Seems the coverage is broken.
/wait

[tyxia]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the lifetime management of the rate limit quota filter's global client and associated resources by transitioning to shared_ptr and weak_ptr patterns. By introducing a TlsStore to anchor these resources, the changes ensure that the global client and thread-local bucket caches remain valid throughout the filter's lifecycle. Additionally, GlobalRateLimitClientImpl now utilizes std::enable_shared_from_this to safely handle asynchronous tasks on the main dispatcher. Review feedback suggests capturing the dispatcher by reference or retrieving it from the store object within the TlsStore deleter to enhance code clarity and safety.

[gemini-code-assist]

Capturing the dispatcher by pointer &dispatcher is safe here because the main thread dispatcher in Envoy is a long-lived object that outlives the filter factories and filters. However, for clarity and to avoid any potential issues with pointer validity if the context were different, it is recommended to capture the dispatcher by reference or retrieve it from the store object inside the deleter (since TlsStore holds a reference to it). This aligns with the repository guideline that in complex code, it is acceptable to keep cheap safety checks for added safety.

References

1. In complex code, it is acceptable to keep cheap safety checks, even if they appear redundant, for added safety.

[tyxia]

optional change

[tyxia]

Nice change! Thanks @yanavlasov ! Please fix the test coverage failure

✗ source/extensions/filters/http/rate_limit_quota: 96.5% (threshold: 96.6%)

[tyxia]

/retest