-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[opt](stream-load) Implement async future handling for stream load to avoid blocking libevent threads #59428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // use string iequal | ||||||||||||||||||||||||
| #include <event2/buffer.h> | ||||||||||||||||||||||||
| #include <event2/event.h> | ||||||||||||||||||||||||
| #include <event2/http.h> | ||||||||||||||||||||||||
| #include <gen_cpp/FrontendService.h> | ||||||||||||||||||||||||
| #include <gen_cpp/FrontendService_types.h> | ||||||||||||||||||||||||
|
|
@@ -106,14 +107,62 @@ void StreamLoadAction::handle(HttpRequest* req) { | |||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Mark handle as called, indicating all client data has been received | ||||||||||||||||||||||||
| // This is safe to send response now without broken pipe | ||||||||||||||||||||||||
| // Use mutex to protect the critical section and avoid race conditions | ||||||||||||||||||||||||
| std::optional<std::string> response_to_send; | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| std::lock_guard<std::mutex> lock(ctx->response_mutex); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ctx->handle_called = true; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // If using async mode, check if fragment execution has completed | ||||||||||||||||||||||||
| // If fragment completed before handle was called, response info was saved | ||||||||||||||||||||||||
| // Now that handle is called, we can safely send the response | ||||||||||||||||||||||||
| if (ctx->event_base != nullptr && ctx->http_request != nullptr && | ||||||||||||||||||||||||
| ctx->stream_load_action != nullptr) { | ||||||||||||||||||||||||
| // Check if there's a pending response to send | ||||||||||||||||||||||||
| // This happens when fragment execution completed before handle was called | ||||||||||||||||||||||||
| if (ctx->pending_response.has_value()) { | ||||||||||||||||||||||||
| // Fragment execution completed before handle was called | ||||||||||||||||||||||||
| // Now handle is called (data received), safe to send response | ||||||||||||||||||||||||
| response_to_send = std::move(ctx->pending_response.value()); | ||||||||||||||||||||||||
| ctx->pending_response.reset(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Send response outside of lock to avoid holding lock during I/O | ||||||||||||||||||||||||
| if (response_to_send.has_value()) { | ||||||||||||||||||||||||
| HttpChannel::send_reply(req, response_to_send.value()); | ||||||||||||||||||||||||
| _finalize_request_cleanup(ctx); | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // No pending response, fragment execution not completed yet | ||||||||||||||||||||||||
| // Continue with normal handle logic, response will be sent in callback | ||||||||||||||||||||||||
| if (ctx->event_base != nullptr && ctx->http_request != nullptr && | ||||||||||||||||||||||||
| ctx->stream_load_action != nullptr) { | ||||||||||||||||||||||||
| // Continue with normal handle logic, response will be sent in callback | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // status already set to fail | ||||||||||||||||||||||||
| if (ctx->status.ok()) { | ||||||||||||||||||||||||
| ctx->status = _handle(ctx); | ||||||||||||||||||||||||
| ctx->status = _handle(ctx, req); | ||||||||||||||||||||||||
| if (!ctx->status.ok() && !ctx->status.is<PUBLISH_TIMEOUT>()) { | ||||||||||||||||||||||||
| LOG(WARNING) << "handle streaming load failed, id=" << ctx->id | ||||||||||||||||||||||||
| << ", errmsg=" << ctx->status; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // If using async mode and no pending response, response will be sent in callback | ||||||||||||||||||||||||
| if (ctx->event_base != nullptr && ctx->http_request != nullptr && | ||||||||||||||||||||||||
| ctx->stream_load_action != nullptr && ctx->status.ok()) { | ||||||||||||||||||||||||
| // Async mode, response will be sent in callback when fragment execution completes | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Sync mode or fallback mode, continue with original logic | ||||||||||||||||||||||||
| ctx->load_cost_millis = UnixMillis() - ctx->start_millis; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!ctx->status.ok() && !ctx->status.is<PUBLISH_TIMEOUT>()) { | ||||||||||||||||||||||||
|
|
@@ -160,7 +209,7 @@ void StreamLoadAction::handle(HttpRequest* req) { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Status StreamLoadAction::_handle(std::shared_ptr<StreamLoadContext> ctx) { | ||||||||||||||||||||||||
| Status StreamLoadAction::_handle(std::shared_ptr<StreamLoadContext> ctx, HttpRequest* req) { | ||||||||||||||||||||||||
| if (ctx->body_bytes > 0 && ctx->receive_bytes != ctx->body_bytes) { | ||||||||||||||||||||||||
| LOG(WARNING) << "recevie body don't equal with body bytes, body_bytes=" << ctx->body_bytes | ||||||||||||||||||||||||
| << ", receive_bytes=" << ctx->receive_bytes << ", id=" << ctx->id; | ||||||||||||||||||||||||
|
|
@@ -176,26 +225,38 @@ Status StreamLoadAction::_handle(std::shared_ptr<StreamLoadContext> ctx) { | |||||||||||||||||||||||
| RETURN_IF_ERROR(_exec_env->stream_load_executor()->execute_plan_fragment(ctx, mocked)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // wait stream load finish | ||||||||||||||||||||||||
| RETURN_IF_ERROR(ctx->future.get()); | ||||||||||||||||||||||||
| // Check if event_base is set (should be set in _on_header) | ||||||||||||||||||||||||
| // If not set, fallback to sync wait | ||||||||||||||||||||||||
| if (ctx->event_base == nullptr || ctx->http_request == nullptr || | ||||||||||||||||||||||||
| ctx->stream_load_action == nullptr) { | ||||||||||||||||||||||||
| // event_base not set (should not happen), fallback to sync wait | ||||||||||||||||||||||||
| LOG(WARNING) << "event_base not set, fallback to sync wait, ctx=" << ctx->id.to_string(); | ||||||||||||||||||||||||
| RETURN_IF_ERROR(ctx->future.get()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (ctx->group_commit) { | ||||||||||||||||||||||||
| LOG(INFO) << "skip commit because this is group commit, pipe_id=" << ctx->id.to_string(); | ||||||||||||||||||||||||
| // Continue with original logic | ||||||||||||||||||||||||
| if (ctx->group_commit) { | ||||||||||||||||||||||||
| LOG(INFO) << "skip commit because this is group commit, pipe_id=" | ||||||||||||||||||||||||
| << ctx->id.to_string(); | ||||||||||||||||||||||||
| return Status::OK(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (ctx->two_phase_commit) { | ||||||||||||||||||||||||
| int64_t pre_commit_start_time = MonotonicNanos(); | ||||||||||||||||||||||||
| RETURN_IF_ERROR(_exec_env->stream_load_executor()->pre_commit_txn(ctx.get())); | ||||||||||||||||||||||||
| ctx->pre_commit_txn_cost_nanos = MonotonicNanos() - pre_commit_start_time; | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| int64_t commit_and_publish_start_time = MonotonicNanos(); | ||||||||||||||||||||||||
| RETURN_IF_ERROR(_exec_env->stream_load_executor()->commit_txn(ctx.get())); | ||||||||||||||||||||||||
| ctx->commit_and_publish_txn_cost_nanos = | ||||||||||||||||||||||||
| MonotonicNanos() - commit_and_publish_start_time; | ||||||||||||||||||||||||
| g_stream_load_commit_and_publish_latency_ms | ||||||||||||||||||||||||
| << ctx->commit_and_publish_txn_cost_nanos / 1000000; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return Status::OK(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (ctx->two_phase_commit) { | ||||||||||||||||||||||||
| int64_t pre_commit_start_time = MonotonicNanos(); | ||||||||||||||||||||||||
| RETURN_IF_ERROR(_exec_env->stream_load_executor()->pre_commit_txn(ctx.get())); | ||||||||||||||||||||||||
| ctx->pre_commit_txn_cost_nanos = MonotonicNanos() - pre_commit_start_time; | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| // If put file success we need commit this load | ||||||||||||||||||||||||
| int64_t commit_and_publish_start_time = MonotonicNanos(); | ||||||||||||||||||||||||
| RETURN_IF_ERROR(_exec_env->stream_load_executor()->commit_txn(ctx.get())); | ||||||||||||||||||||||||
| ctx->commit_and_publish_txn_cost_nanos = MonotonicNanos() - commit_and_publish_start_time; | ||||||||||||||||||||||||
| g_stream_load_commit_and_publish_latency_ms | ||||||||||||||||||||||||
| << ctx->commit_and_publish_txn_cost_nanos / 1000000; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| // event_base is set, use async mode, return directly | ||||||||||||||||||||||||
| // Future completion will trigger callback in stream_load_executor | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| // Future completion will trigger callback in stream_load_executor | |
| // stream_load_executor will be invoked via event_base_once (zero-timeout) callback |
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer dereference: In continue_handle_after_future, ctx->http_request is accessed on lines 996, 1001, 1013, 1020, and 1038, but it can be set to nullptr in free_handler_ctx (line 441). Even though there's a check on line 74 of stream_load_executor.cpp, there's no guarantee that http_request won't become null between that check and when continue_handle_after_future is called, creating a potential crash.
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated rollback logic: The rollback logic appears in multiple places - in continue_handle_after_future (lines 995, 1010) and in _finalize_request (lines 1046). This duplication could lead to maintenance issues if the rollback behavior needs to change. Additionally, there's a risk of double rollback if both code paths execute, though the need_rollback flag should prevent this in most cases.
| if (ctx->need_rollback) { | |
| _exec_env->stream_load_executor()->rollback_txn(ctx.get()); | |
| ctx->need_rollback = false; | |
| } | |
| auto rollback_if_needed = [this](const std::shared_ptr<StreamLoadContext>& c) { | |
| if (c->need_rollback) { | |
| _exec_env->stream_load_executor()->rollback_txn(c.get()); | |
| c->need_rollback = false; | |
| } | |
| }; | |
| rollback_if_needed(ctx); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| #pragma once | ||
|
|
||
| #include <event2/event.h> | ||
| #include <gen_cpp/BackendService_types.h> | ||
| #include <gen_cpp/FrontendService_types.h> | ||
| #include <gen_cpp/PlanNodes_types.h> | ||
|
|
@@ -27,6 +28,7 @@ | |
| #include <future> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
@@ -47,6 +49,10 @@ namespace io { | |
| class StreamLoadPipe; | ||
| } // namespace io | ||
|
|
||
| // Forward declarations | ||
| class HttpRequest; | ||
| class StreamLoadAction; | ||
|
|
||
| // kafka related info | ||
| class KafkaLoadInfo { | ||
| public: | ||
|
|
@@ -256,6 +262,19 @@ class StreamLoadContext { | |
| std::string qualified_user; | ||
| std::string cloud_cluster; | ||
|
|
||
| // Fields for async processing (Scheme B: libevent deferred callback) | ||
| // These fields are set in _on_header() before execute_plan_fragment is called | ||
| // to avoid race conditions | ||
| struct event_base* event_base = nullptr; // libevent event loop | ||
| HttpRequest* http_request = nullptr; // HTTP request reference | ||
| StreamLoadAction* stream_load_action = nullptr; // StreamLoadAction instance pointer | ||
|
Comment on lines
+268
to
+270
|
||
| bool handle_called { | ||
| false}; // Flag to indicate if handle() has been called (data received completely) | ||
| std::optional<std::string> | ||
| pending_response; // Pending response string to send when handle() is called | ||
| mutable std::mutex | ||
| response_mutex; // Mutex to protect pending_response and handle_called check/set | ||
|
|
||
| public: | ||
| ExecEnv* exec_env() { return _exec_env; } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "recevie" should be spelled "receive".