Uh oh!
There was an error while loading. Please reload this page.
Fix connection leak by request timers not cancelled in time - #555
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #554 by refactoring how ClientConnection tracks in-flight requests and their timeouts, aiming to avoid connection objects being kept alive by outstanding timer handlers.
Changes:
- Introduces a
PendingRequest<T>helper that owns a timer + promise and usesweak_from_this()in the timer handler. - Adds
ExecutorService::createTimer()to construct anASIO::steady_timerwith an expiry. - Migrates multiple
ClientConnectionpending-request maps (requests/lookups/stats/schema/etc.) to storePendingRequestinstances and removes the old consumer-stats sweep timer logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
lib/PendingRequest.h | New helper abstraction for request promise + timeout timer. |
lib/ExecutorService.h | Adds a createTimer() convenience factory for steady_timer. |
lib/ClientConnection.h | Replaces several pending-request structs/maps with PendingRequest-based maps (mostly unordered_map). |
lib/ClientConnection.cc | Switches request creation/timeout wiring to PendingRequest, removes consumer-stats periodic timer cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#554
Motivation
After a pending
GetLastMessageIdrequest is removed frompendingGetLastMessageIdRequests_(e.g. inhandleGetLastMessageIdResponse), the associated timer is not cancelled, so the callback, which captures theshared_from_this()ofClientConnection, still exists in the executor's internal queue. We should cancel the timer when it's removed.Modifications
Add a
PendingRequestabstraction to replace the manually written struct of the promise and timer, then cancel the timer in its destructor.Add
ClientTest::testConnectionNotReferredAfterCloseto verify the change works that the close time won't be too long. AddtestTimedOutPendingRequestsAreErasedFromConnectionMapsto verify the timeout logic is handled well inPendingRequest.