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.
Последствия досрочного выхода из этого цикла хуже, чем бесконечное ожидание. И тут никогда не может быть таймаут, обработка таймаута создаст ложное впечатление, что он возможен
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // FIXME: NOT SAFE (see man libcurl(3)) | ||
| const CURLcode globalInitResult = curl_global_init(CURL_GLOBAL_ALL); |
There was a problem hiding this comment.
The FIXME comment indicates a known safety issue with curl_global_init but doesn't explain what the problem is or how it should be fixed. Consider documenting that curl_global_init is not thread-safe and should be called only once before any threads are started, or reference the specific safety concerns from the libcurl documentation.
| if (multiCleanupResult != CURLM_OK) { | ||
| Cerr << "curl_multi_cleanup error " << int(multiCleanupResult) << ": " << curl_multi_strerror(multiCleanupResult) << Endl; | ||
| } | ||
| curl_global_cleanup(); // FIXME: NOT SAFE (see man libcurl(3)) |
There was a problem hiding this comment.
The FIXME comment indicates a known safety issue with curl_global_cleanup but doesn't explain what the problem is. Consider documenting that curl_global_cleanup must be called only after all libcurl operations have completed and all handles are destroyed, and it's not thread-safe. This is particularly important since the cleanup is now in a shared_ptr deleter.
| curl_global_cleanup(); // FIXME: NOT SAFE (see man libcurl(3)) | |
| // WARNING: curl_global_cleanup() must only be called after all libcurl operations have completed | |
| // and all easy and multi handles have been destroyed. This function is not thread-safe and must not | |
| // be called concurrently with any other libcurl function. Since this cleanup is performed in the | |
| // shared_ptr deleter, it is critical to ensure that no other libcurl handles are in use and no other | |
| // threads are using libcurl when this is called. See man libcurl(3) and the libcurl documentation for details. | |
| curl_global_cleanup(); |
| auto oldSize = Counter->fetch_sub(size()); | ||
| if (oldSize >= Threshold && oldSize - size() < Threshold) { |
There was a problem hiding this comment.
There's a potential race condition here. Between fetch_sub and checking the threshold, another thread could also call fetch_sub, causing multiple wakeup calls even though only one thread actually crossed the threshold. While extra wakeups are generally harmless, consider if this could cause performance issues under high concurrency. If so, consider using compare-and-exchange to ensure only one thread triggers the wakeup when crossing the threshold.
| auto oldSize = Counter->fetch_sub(size()); | |
| if (oldSize >= Threshold && oldSize - size() < Threshold) { | |
| size_t expected = Counter->load(); | |
| bool triggered = false; | |
| while (true) { | |
| if (expected < Threshold || expected - size() >= Threshold) { | |
| // No threshold crossing, just subtract | |
| if (Counter->compare_exchange_weak(expected, expected - size())) { | |
| break; | |
| } | |
| // compare_exchange_weak updates expected, so loop | |
| } else { | |
| // Threshold crossing, only one thread should trigger | |
| if (Counter->compare_exchange_weak(expected, expected - size())) { | |
| triggered = true; | |
| break; | |
| } | |
| // compare_exchange_weak updates expected, so loop | |
| } | |
| } | |
| if (triggered) { |
iddqdex
added
the
rebase-and-check
github-actions
bot
removed
the
rebase-and-check
|
⚪ ⚪ 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 |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
…tform#28390) (cherry picked from commit c734e91)
…tform#28390) (cherry picked from commit c734e91)
4 participants
There can be some spurious/double wakeup, but this should not matter much
Changelog entry
...
Changelog category
Description for reviewers
...