[fix](rpc) Fix AutoReleaseClosure data race with callback reuse - #61782
Conversation
|
/review |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Pull request overview
Fixes a brpc AutoReleaseClosure race when callbacks are reused to start a new RPC (mutating response_/cntl_), by ensuring status/failure checks happen before invoking callback->call(), and adds unit tests to prevent regressions.
Changes:
- Reorders
AutoReleaseClosure::Run()so it checkscntl_/response_->status()before invokingcallback->call(). - Refactors runtime-filter sync-size RPC handling to keep the callback alive until RPC completion.
- Adds BE unit tests that simulate callback reuse mutating shared RPC state.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| be/test/exec/exchange/exchange_sink_test.cpp | Adds tests that simulate callback reuse and verify correct ordering. |
| be/src/util/brpc_closure.h | Reorders Run() and simplifies error handling paths. |
| be/src/exec/runtime_filter/runtime_filter_producer.h | Adds storage to keep sync-size callback alive across async RPC. |
| be/src/exec/runtime_filter/runtime_filter_producer.cpp | Introduces SyncSizeCallback and changes closure construction/lifetime. |
| be/src/exec/runtime_filter/runtime_filter_mgr.h | Updates _send_rf_to_target signature (removes QueryContext arg). |
| be/src/exec/runtime_filter/runtime_filter_mgr.cpp | Updates runtime-filter RPC closure construction (drops ctx passing). |
| be/src/exec/runtime_filter/runtime_filter.cpp | Updates runtime-filter RPC closure construction (drops ctx passing). |
| be/src/exec/operator/exchange_sink_buffer.cpp | Adds comments documenting callback reuse ordering constraints. |
| be/src/exec/exchange/vdata_stream_sender.h | Documents callback reuse rationale. |
Comments suppressed due to low confidence (1)
be/src/util/brpc_closure.h:1
- This change removes the previous
QueryContext-based failure propagation (anderror_msgaugmentation) fromAutoReleaseClosure, replacing it with logging only. That’s a behavior change: some call sites previously relied onAutoReleaseClosureto cancel the query on RPC failure / non-OKstatus(), gated byignore_runtime_filter_error; after this patch, errors may no longer interrupt the query and could lead to hangs or delayed failure handling. Recommendation (mandatory): restore a mechanism to propagate failures (either by reintroducing the optionalQueryContext+ captured status/failure info before callingtmp->call(), or by requiring callers to pass a callback that performs the cancel/sub/error handling) while keeping thecall()invocation as the last step to avoid the reuse race.
// Licensed to the Apache Software Foundation (ASF) under one
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
PR Goal & Accomplishment
This PR fixes a real data race in AutoReleaseClosure::Run() where call() was invoked before checking cntl_->Failed() and response_->status(). Because call() can trigger callback reuse (e.g., in ExchangeSendCallback::call() -> _suc_fn -> _send_rpc() which reuses the same callback via get_send_callback()), the closure would then read mutated cntl_/response_ state from the new RPC instead of the original. The fix correctly reorders: log/check first, then call() last. The crash stacktrace and debug-log evidence clearly confirm the race.
The secondary change — removing QueryContext cancellation from AutoReleaseClosure and moving error handling into individual callbacks — is also sound. All callback types that need error handling (ExchangeSendCallback, SyncSizeCallback, WriteBlockCallback) already fully handle errors in their call() methods. The DummyBrpcCallback paths either have caller-side join()+check patterns or are fire-and-forget operations where query cancellation was too aggressive.
Compilation Bug Found
The old code had two _process_status overloads: an unconstrained no-op fallback template <typename Response> void _process_status(Response*) {} and a constrained template <HasStatus Response> void _process_status(Response*). The new code only has the constrained _log_error_status<HasStatus Response> but removed the unconstrained fallback. This will cause a compilation failure for PTabletWriterCancelResult (an empty protobuf message with no status() field), used in vtablet_writer.cpp:1229.
Critical Checkpoint Conclusions
-
Goal accomplished? Yes, the core data race fix is correct. Tests prove the ordering. However, there is a compilation bug (see inline comment).
-
Modification minimal and focused? Yes. The changes are well-scoped: core fix in
brpc_closure.h, consequent cleanup ofQueryContextremoval, andSyncSizeClosure->SyncSizeCallbackrefactor. -
Concurrency: The core fix addresses the concurrency issue correctly by ensuring
call()(which may trigger callback reuse and new RPCs) is the last operation inRun(). TheSyncSizeCallback::call()readscntl_andresponse_which are safe becausecall()is now called before any reuse can happen. -
Lifecycle management:
_sync_size_callback(asshared_ptr<void>) correctly keeps theSyncSizeCallbackalive until theRuntimeFilterProduceris destroyed. Without this,AutoReleaseClosure'sweak_ptrto the callback would expire whensend_size()returns, and the error-pathsub()would never fire. This is correct. -
Parallel code paths: The two success handler lambdas in
exchange_sink_buffer.cpp(unicast and broadcast) both have the same comment added about_send_rpcbeing last. Good. -
Test coverage: Three well-designed unit tests verify the ordering invariant by capturing state at
call()time and mutating afterward. Tests correctly fail with the old ordering and pass with the new. -
Error handling: The behavioral change from
QueryContext::cancel()toLOG(WARNING)is safe — all critical paths already handle errors in their callbacks or via caller-sidejoin()+check. -
No incompatible changes: No function symbol or storage format changes.
-
Configuration: No new configs added.
-
Observability: Error logging is preserved (downgraded from
canceltoLOG(WARNING)inAutoReleaseClosure, but callbacks handle errors appropriately).
|
/review |
|
run buildall |
There was a problem hiding this comment.
Code Review Summary
PR Goal
Fix a real data race in AutoReleaseClosure where call() was invoked before checking cntl_->Failed() and response_->status(). When call() reuses the callback (as in ExchangeSendCallback via get_send_callback()), it can trigger a new RPC that mutates response_ and cntl_, causing the subsequent reads to see the new RPC's state instead of the original — leading to SIGSEGV crashes.
Core Fix Assessment
The fix is correct and well-motivated:
- Reordering in
Run(): Log/check cntl/response state before callingcallback_->call(). This prevents the data race. - Removing
QueryContextfromAutoReleaseClosure: Decentralizes error handling to individual callbacks, which is architecturally cleaner. SyncSizeCallbackrefactoring: Moves from subclassingAutoReleaseClosureto subclassingDummyBrpcCallback, properly handling errors incall().- Unit tests: Three well-designed tests verify the ordering invariant.
Critical Checkpoint Conclusions
- Goal accomplished? Yes, the data race is fixed by reordering
call()to be last inRun(). Tests prove the ordering. - Modification minimal and focused? Mostly yes. The
QueryContextremoval is a secondary cleanup bundled with the fix. - Concurrency? The core race condition is correctly addressed. The callback reuse pattern in
ExchangeSendCallbackis safe because_send_rpc(which triggers reuse) is documented and enforced as the last operation in success handlers. - Lifecycle management?
_sync_size_callbackasshared_ptr<void>correctly keeps the callback alive. Theweak_ptrinAutoReleaseClosureallows graceful degradation if the callback owner is gone. - Parallel code paths? Both unicast and broadcast paths in
exchange_sink_buffer.cppare updated with matching comments. - Error handling? See inline comment — the
ignore_runtime_filter_error=false+ RPC failure path that previously cancelled the query now silently degrades. The existing regression testtest_inject_send_filter_size_fail.groovyexpects the old cancellation behavior and will likely fail. - Test coverage? Good unit tests for the ordering fix. Missing update to the regression test.
- Incompatible changes? No storage/protocol changes. The
AutoReleaseClosureconstructor signature change is source-breaking but all callers are updated. - Performance? No concerns — the reordering adds no overhead.
TPC-H: Total hot run time: 26928 ms |
TPC-DS: Total hot run time: 167957 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Code Review Summary
PR Goal
Fix a data race in AutoReleaseClosure::Run() where call() was invoked before checking cntl_->Failed() and response_->status(). Since call() can reuse the callback (resetting cntl_ and response_), subsequent reads of these fields in Run() would see the NEW RPC's state instead of the original, causing SIGSEGV crashes.
Core Fix Assessment
The fix is correct and well-motivated. Moving callback->call() to be the last operation in AutoReleaseClosure::Run() (after logging) eliminates the data race. The approach of removing QueryContext from AutoReleaseClosure and pushing error handling into individual callbacks is a clean architectural improvement that makes the responsibilities clearer.
Critical Checkpoint Conclusions
1. Does the code accomplish its goal?
Yes. The reordering in AutoReleaseClosure::Run() ensures cntl_ and response_ are read before call() can mutate them. The new tests prove the ordering.
2. Is the modification minimal and focused?
Mostly yes. The core fix (reordering in brpc_closure.h) is minimal. The SyncSizeCallback refactoring is a necessary consequence. However, the removal of QueryContext-based error propagation from the DummyBrpcCallback paths (merge_filter, sync_filter_size coordinator-side, publish_filter) is a behavioral side-effect that goes beyond the race fix -- see inline comment.
3. Concurrency?
Yes, this is fundamentally a concurrency fix. The critical variable is the shared cntl_/response_ state between AutoReleaseClosure and the callback. The fix ensures the closure reads before the callback writes, without requiring locks (sequential ordering within Run() suffices since the race was between the closure's own read and the callback's write-then-reuse pattern).
4. Lifecycle management?
The _sync_size_callback member in RuntimeFilterProducer correctly keeps the SyncSizeCallback alive beyond send_size() return, compensating for AutoReleaseClosure's weak_ptr storage. Good design.
5. Parallel code paths?
WriteBlockCallback (in vtablet_writer.h) follows the same pattern and is safe: its call() method handles errors internally and calls clear_in_flight() at the very end. The ExchangeSendCallback also handles errors internally in call(). Both are compatible with the new ordering.
6. Test coverage?
Good. Three new tests in exchange_sink_test.cpp verify the ordering invariant. Seven tests in sync_size_callback_test.cpp cover success, failure, ignore_runtime_filter_error, and expired weak_ptr scenarios for SyncSizeCallback.
7. Configuration changes? None.
8. Incompatible changes? None (internal refactoring only).
9. Performance? No concerns. The _log_error_status call before callback->call() is lightweight (just a LOG on error).
10. Behavioral change concern: See inline comment on runtime_filter.cpp. The removal of QueryContext from AutoReleaseClosure means that merge_filter, coordinator-side sync_filter_size, and publish_filter RPC failures no longer cancel the query when ignore_runtime_filter_error=false. This is a behavioral change beyond the race fix scope.
11. Stale comment: runtime_filter_wrapper.h:91 still references SyncSizeClosure (now renamed to SyncSizeCallback). Not in the diff, so noting here for follow-up.
Verdict
The core race fix is correct and well-tested. One behavioral concern is flagged inline regarding the loss of query cancellation on runtime filter RPC failures in non-SyncSizeCallback paths.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/review |
There was a problem hiding this comment.
Findings:
-
be/src/exec/runtime_filter/runtime_filter_mgr.cpp:432now keepsTrivialBrpcCallbackowners alive inpublish_callbacksuntilGlobalMergeContext::reset(). Each callback owns itsbrpc::Controller, and in this path that controller owns the serialized runtime-filter payload viarequest_attachment(). Before this refactor the controller died with the closure afterRun(). Now a large bloom filter can stay pinned for the rest of the query/round even after the RPC has completed, which is a material memory regression on global runtime-filter workloads. -
be/src/exec/runtime_filter/runtime_filter.cpp:39has the same retention problem for merge RPCs._merge_filter_callbackkeeps the callback/controller alive on theRuntimeFilterobject, and_push_to_remote()appends the serialized filter bytes into that controller's attachment. Because nothing resets the controller afterRun(), the merged filter payload now lives until the producer/merger object is destroyed instead of until the RPC finishes.
Critical checkpoint conclusions:
- Goal of the task: fix the callback-reuse data race. The PR does address that race and adds targeted BE unit tests, but I cannot sign off because the current ownership fix introduces a new end-to-end memory-lifetime regression in runtime-filter RPC paths.
- Small / clear / focused: not fully. The change refactors generic callback ownership semantics and touches many call sites beyond the original race site.
- Concurrency: the original read-after-reuse race is addressed. I did not find a new lock-order or deadlock issue in the touched code.
- Lifecycle management: this is the main problem area. Long-lived callback owners now also keep
brpc::Controllerattachments alive after callback completion. No static-initialization-order issue found. - Configuration changes: none.
- Compatibility / incompatible changes: none found.
- Parallel code paths: the main runtime-filter send/publish paths were updated consistently; synchronous join-based paths still look correct.
- Special conditional checks: acceptable in the touched code.
- Test coverage: good targeted unit coverage for callback reuse behavior; I did not see coverage for post-callback cleanup / controller lifetime.
- Test result files: not applicable.
- Observability: sufficient for these paths.
- Transaction / persistence / data-write correctness: not applicable.
- Performance: blocking memory-retention regression in runtime-filter RPC paths.
- Other issues: none beyond the blocking items above.
| DummyBrpcCallback<PSyncFilterSizeResponse>::create_shared(), ctx); | ||
| auto callback = TrivialBrpcCallback<PSyncFilterSizeResponse>::create_shared( | ||
| query_ctx->ignore_runtime_filter_error() ? std::weak_ptr<QueryContext> {} | ||
| : query_ctx->weak_from_this()); |
There was a problem hiding this comment.
weak_ptr。跟以前语义没改变。
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Issue Number: None
Related PR: None
Problem Summary: Switch AutoReleaseClosure to shared callback ownership by default, require explicit weak ownership opt-in, remove ManagedTrivialBrpcCallback-based owner containers where strong closure ownership is sufficient, and keep weak ownership only for callbacks with external lifecycle dependencies.
None
- Test: ./build.sh --be -j60
- Unit Test: ./run-be-ut.sh --run --filter=*SyncSizeCallback*:*ExchangeSink* -j60
- Behavior changed: Yes (AutoReleaseClosure now strongly owns trivial callbacks by default; weak ownership is explicit)
- Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
No blocking findings.
Critical checkpoints:
- Goal of current task: Fix the
AutoReleaseClosurerace when a callback object is reused for a subsequent RPC before the old closure finishes readingcntl_/response_. The patch addresses that by moving post-RPC handling into callback-ownedcall()implementations and by making callback lifetime explicit at the affected async call sites. The changed code matches the stated goal. - Is the modification small, clear, and focused: Mostly yes. The scope is broader than a local fix because it changes a shared utility contract, but the follow-up adjustments are concentrated in the exchange and runtime-filter paths that depend on callback reuse / async ownership.
- Concurrency: Applicable. The relevant concurrency is brpc completion bthreads racing with callback reuse on exchange and runtime-filter RPC objects. The new
Run()implementation no longer readscntl_/response_aftercall(), which removes the reported race. I did not find a new lock-order or heavy-under-lock regression in the touched code. - Lifecycle / static initialization: Applicable for callback ownership.
AutoReleaseClosurenow only keeps a weak reference, so callback owners must be explicit. The changed async call sites that need callback side effects now retain shared owners (_send_callback,_merge_filter_callback,_sync_size_callback,sync_size_callbacks,publish_callbacks). No static initialization issue found. - Configuration changes: None.
- Incompatible changes / compatibility: No FE/BE protocol, symbol, or storage-format compatibility issue found.
- Parallel code paths: Reviewed other
AutoReleaseClosureusers. The unchanged synchronous paths still keep callbacks alive untiljoin(), and the reused-callback async paths were updated consistently. - Special conditional checks/comments: The new comments in exchange sink about
_send_rpcneeding to be the last operation are warranted because the callback reuse requirement is non-obvious. - Test coverage: Partially sufficient. The new BE unit tests cover the runtime-filter callback behavior and the weak-ownership contract. I did not see the exchange-sink regression test mentioned in the PR description in the actual diff, so the originally reported repro path still has some residual test-gap risk.
- Test result modifications: Not applicable.
- Observability: Existing warning logs are sufficient for the touched RPC failure/status paths; no additional metrics appear necessary.
- Transaction / persistence / data-write correctness: Not applicable to this change.
- FE-BE variable propagation: Not applicable.
- Performance: The change preserves callback reuse on the hot exchange path and removes the racy post-callback status read. No obvious new redundant work or allocation issue stood out.
- Other issues: None blocking found in the reviewed paths.
Overall opinion: the PR quality is good and I do not see a blocking correctness regression in the landed change. The only notable residual risk is that the exchange-sink repro path described in the PR body is not covered by a new test in this diff.
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
The callback's call() method may reuse the callback object (e.g., in vdata_stream_sender.h get_send_callback()), triggering a new RPC that mutates response_ and cntl_. If AutoReleaseClosure::Run() invokes call() before checking cntl_->Failed() or response_->status(), it reads the NEW RPC's state instead of the ORIGINAL RPC's result, causing:
we have confirmed the data race is real existing with temporary LOGs which has been removed:
and we add some be-ut which could only pass WITH this patch.
before we fix:
after: