Fix outstanding requests overcount with a single atomic counter - #2583
Fix outstanding requests overcount with a single atomic counter#2583harshal24-chavan wants to merge 1 commit into
Conversation
|
@harshal24-chavan thanks for this PR. The single-counter approach looks correct and I don't see an obvious lifecycle/double-decrement issue. However, I don't think requestsBufferSize() should be removed from the public HttpClient API in this refactor. Removing it is a source-breaking API change, while the counter consolidation does not require removing the API. requestsBufferSize() can still be implemented from requestsBuffer_.size() since the queue is accessed on the event-loop thread. Please consider keeping the existing API for backward compatibility and add explicit tests for timeout and connection-error paths to verify that every request is decremented exactly once. |
|
Thanks for the review @an-tao. Understood, I'll revert back the requestsBufferSize() function changes, and update the tests. |
What does this PR do?
This PR refactors
HttpClientImplto use a singlestd::atomic<std::size_t>to track outstanding requests, replacing the previous two-counter system (requestsBufferSize_andpipeliningCallbacksSize_).Why is this needed?
Previously, reading
outstandingRequests()could yield inconsistent snapshots. BecauserequestsBuffer_andpipeliningCallbacks_were tracked by independent atomics, which could lead to undercount / overcount of the total requests.How does this fix it?
By transitioning to a single lifecycle-based counter:
outstandingRequests_.fetch_add(1)is called exactly once when a request enterssendRequestInLoop.outstandingRequests_.fetch_sub(1)is called when a request is fully completed, aborted, or times out.This completely eliminates the undercount / overcount, makes the tracking mathematically sound regardless of thread timing, and allowed for the removal of several redundant queue helper functions, simplifying the class API.
Changes:
outstandingRequests_.enqueueRequest,popFrontRequest, anderaseRequesthelpers.requestsBufferSize()from the publicHttpClientAPI (updated example and test to match).