http_gateway: wake up poll when we released inflight buffers #28390
Conversation
There can be some spurious/double wakeup, but this should not matter much
|
🟢 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the HTTP gateway implementation to improve resource management and implement a wake-up mechanism for efficient buffer handling. The key change is converting the CURL multi-handle from a raw pointer to a std::shared_ptr to enable proper lifetime tracking.
Key Changes:
- Convert
CURLM*handle tostd::shared_ptr<CURLM>with custom deleter for automatic cleanup - Add
handleandthresholdparameters toTCountedContentto trigger wake-ups when buffered data falls below threshold - Implement automatic wake-up logic in
TCountedContentdestructor andExtract()method when buffer size crosses threshold
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ydb/library/yql/providers/common/http_gateway/yql_http_gateway.h | Updated TCountedContent constructor signature and added Handle and Threshold member variables |
| ydb/library/yql/providers/common/http_gateway/yql_http_gateway.cpp | Converted CURL multi-handle to shared_ptr, updated all handle usage sites to call .get(), implemented wake-up logic in TCountedContent, and added busy-wait synchronization in cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| auto weakHandle = std::weak_ptr<CURLM>(Handle); | ||
| Handle.reset(); | ||
| while (!weakHandle.expired()) {} // short busy-wait in case of TCountedContent |
There was a problem hiding this comment.
This busy-wait loop is problematic. It will consume 100% CPU while waiting for all TCountedContent instances to be destroyed. Consider using a more efficient synchronization mechanism like a condition variable, or at least add a small sleep/yield inside the loop to reduce CPU usage.
auto weakHandle = std::weak_ptr<CURLM>(Handle);
Handle.reset();
std::this_thread::yield();
while (!weakHandle.expired()) {
std::this_thread::sleep_for(std::chrono::microseconds(100));
}| while (!weakHandle.expired()) {} // short busy-wait in case of TCountedContent | |
| while (!weakHandle.expired()) { | |
| std::this_thread::sleep_for(std::chrono::microseconds(100)); | |
| } |
There was a problem hiding this comment.
In general, yes, busy-loops are bad; however, in this case it is expected to happen only on destruction (already rare!), only in cases when there are concurrently executed move-out or destruction from TCountedContent, and only for very short period (when curl_multi_wakeup is executed)
Hence, comparing required expenses and complications from better synchronization primitives, the choice is pretty obvious.
| if (oldSize >= Threshold && oldSize - size() < Threshold) { | ||
| if (auto handle = Handle.lock()) { | ||
| curl_multi_wakeup(handle.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The threshold check logic has a potential race condition. Between checking oldSize >= Threshold and evaluating oldSize - size() < Threshold, another thread could modify the Counter, making the condition incorrect. This is compounded by the fact that oldSize is the value after the subtraction (fetch_sub returns the previous value), so oldSize - size() is actually the new value after this operation.
However, there's a more fundamental issue: if Threshold is 0 (the default), the condition oldSize >= Threshold && oldSize - size() < Threshold would trigger on every destruction where size() > 0, potentially causing excessive wake-up calls. Consider adding a check: if (Threshold > 0 && oldSize >= Threshold && oldSize - size() < Threshold)
There was a problem hiding this comment.
Counter is sum of all size(), hence both should not be issue.
Anyway, rare spurious (or even missed) wake-up should not be issue.
| if (oldSize >= Threshold && oldSize - size() < Threshold) { | ||
| if (auto handle = Handle.lock()) { | ||
| curl_multi_wakeup(handle.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as in the destructor: if Threshold is 0 (the default), the condition oldSize >= Threshold && oldSize - size() < Threshold would trigger on every extraction where size() > 0, potentially causing excessive wake-up calls. Consider adding a check: if (Threshold > 0 && oldSize >= Threshold && oldSize - size() < Threshold)
|
⚪ |
|
⚪ |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| auto weakHandle = std::weak_ptr<CURLM>(Handle); | ||
| Handle.reset(); | ||
| while (!weakHandle.expired()) { // short busy-wait in unlikely case of collision with TCountedContent | ||
| Sleep(TDuration::MicroSeconds(1)); |
There was a problem hiding this comment.
путь к зависаниям. давай таймаут сделаем
There was a problem hiding this comment.
Последствия досрочного выхода из этого цикла хуже, чем бесконечное ожидание. И тут никогда не может быть таймаут, обработка таймаута создаст ложное впечатление, что он возможен
2 participants
There can be some spurious/double wakeup, but this should not matter much
Changelog entry
...
Changelog category
Description for reviewers
...