Conversation
Reviewer's GuideFixes native server shutdown ordering by draining the HTTP server CPU executor before destroying the driver executor, adds a regression test for the use-after-free scenario, and optimizes sidecar result serialization by moving the result into JSON conversion. Sequence diagram for safe native server shutdown orderingsequenceDiagram
participant Server as PrestoServer
participant HttpCpu as httpSrvCpuExecutor_
participant Query as HTTP task /v1/expressions
participant Driver as driverExecutor_
Server->>HttpCpu: join()
HttpCpu->>Query: Finish queued CPU work
Query->>Driver: Use driverExecutor_.get() via QueryCtx
Driver-->>Query: Complete dispatched work
HttpCpu-->>Server: HTTP CPU executor drained
Server->>Driver: reset() / stop driver executor
Note over Server,Driver: Driver executor is destroyed only after HTTP tasks finish
Sequence diagram for sidecar result serializationsequenceDiagram
participant Sidecar as Sidecar endpoint
participant Cpu as httpSrvCpuExecutor_
participant Json as JSON serializer
Sidecar->>Cpu: thenValue(result)
Cpu->>Json: json(std::move(result))
Json-->>Cpu: Serialized JSON bytes
Cpu-->>Sidecar: dumpJson result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
286840e to
ee3a0a3
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The regression test hard-codes the expected order and would pass if the production fix were reverted.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Correct order: drain httpSrvCpu before destroying driverExecutor. | ||
| httpSrvCpu->join(); |
There was a problem hiding this comment.
Thats how the above test cases do it as well in ShutdownOrderTest so I think its okay to test it like this for now.
|
@amitkdutta : Hi Amit, Can you take a look at this PR as well since its in the main PrestoServer.cpp path ? |
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Reviewed for shutdown/lifetime safety. The use-after-free this fixes is real, but reordering joinExecutors() does not fully resolve it — httpSrvCpuExecutor_ has a dependency in the opposite direction too, so the new order trades one hazard for another. Details inline; folly behaviour below is from the vendored copy under presto-native-execution/folly.
Summary of findings:
httpSrvCpuExecutor_is TaskManager's continuation executor, so joining it before the driver pool inverts a second, opposing constraint (inline onPrestoServer.cpp).- The keepalive wait inside
join()now runs first, moving the shutdown-hang risk to the front ofjoinExecutors()with nothing else drained (inline onPrestoServer.cpp). - The regression test does not pin the production ordering (inline on
ShutdownOrderTest.cpp) — agreeing with Copilot's comment here. json(std::move(result))is safe but not applied to the sibling handler (inline onPrestoServer.cpp).
Nothing here blocks on its own; finding 1 is the one worth resolving before merge, because ordering alone cannot satisfy both directions of the dependency.
| << "': threads: " << httpSrvCpuExecutor_->numActiveThreads() << "/" | ||
| << httpSrvCpuExecutor_->numThreads() | ||
| << ", task queue: " << httpSrvCpuExecutor_->getTaskQueueSize(); | ||
| httpSrvCpuExecutor_->join(); |
There was a problem hiding this comment.
reordering creates the mirror-image hazard
httpSrvCpuExecutor_ is not only the sidecar endpoints' executor. It is also TaskManager's and TaskResource's continuation executor: TaskManager.cpp lines 1104, 1122, 1143, 1157, 1179, 1186, 1213, 1228, 1251, 1294, 1314, 1327 all do .via(httpSrvCpuExecutor_). In particular prestoTask->task->stateChangeFuture(maxWaitMicros).via(httpSrvCpuExecutor_) (TaskManager.cpp:1143) is fulfilled by a driver thread on task state change.
So the dependency runs both ways:
- sidecar tasks on httpSrvCpu →
driverExecutor_(the direction this PR fixes), and - driver threads → continuations scheduled onto httpSrvCpu (the direction this PR breaks).
Joining httpSrvCpu here, before driverCpuExecutor_->join() below, means a driver thread can touch an already-joined pool. What folly does in that case:
stopAndJoinAllThreadssetsmaxThreads_ = 0andactiveThreads_ = 0(folly/executors/ThreadPoolExecutor.cpp:283-284). A lateradd()enqueues fine, thenensureActiveThreads()seesactive >= totaland starts no thread (ThreadPoolExecutor.cpp:531-536) — the task is silently never run..via(Executor*)acquires a keepalive, andDefaultKeepAliveExecutor::keepAliveAcquireassertsDCHECK(keepAliveCount > 0)(folly/DefaultKeepAliveExecutor.h:141-145). The count is 0 after join, so this fails in debug builds; in release it increments from 0 and the matching release re-postskeepAliveReleaseBaton_.
The only thing making this survivable today is that shutdownServer() destroys httpServer_ and taskManager_ before calling joinExecutors() (lines 947-948, 956). That is not a guarantee: TaskManager::shutdown() waits only on kRunning tasks, and the teardown path explicitly tolerates Velox tasks with outstanding references, logging "Velox task has pending reference on destruction" and continuing (TaskManager.cpp:1540-1555). Driver-side work can therefore still be live when httpSrvCpu is already joined.
Ordering alone cannot satisfy both directions. Suggest removing the raw-pointer dependency instead: hand QueryCtx::create an executor held by a keepalive, or run sidecar expression evaluation on an executor that is not the driver pool. getOptimizedExpressions (lines 224-267) is fully synchronous and only needs an executor for QueryCtx, so it does not need the driver pool specifically.
There was a problem hiding this comment.
@pdabre12 : I'm generally in favor of the suggestion
"Run sidecar expression evaluation on an executor that is not the driver pool. getOptimizedExpressions (lines 224-267) is fully synchronous and only needs an executor for QueryCtx, so it does not need the driver pool specifically."
This change might be changing current problems to something else. If you can implement this suggestion then that works best.
| << "': threads: " << httpSrvCpuExecutor_->numActiveThreads() << "/" | ||
| << httpSrvCpuExecutor_->numThreads() | ||
| << ", task queue: " << httpSrvCpuExecutor_->getTaskQueueSize(); | ||
| httpSrvCpuExecutor_->join(); |
There was a problem hiding this comment.
shutdown-hang risk moves to the front of joinExecutors()
ThreadPoolExecutor::join() calls joinKeepAliveOnce() → joinKeepAlive(), which blocks on keepAliveReleaseBaton_.wait() (folly/DefaultKeepAliveExecutor.h:51-55) until every KeepAlive token on this executor is released. Pending long-poll chains (getTaskInfo/getResults with maxWait) hold exactly those tokens.
Previously this wait ran last, after the driver, connector and exchange CPU pools were already drained. It is now the first thing joinExecutors() does, with every other pool still up and no timeout on the wait. Any token whose release depends on a stage that is joined later turns shutdown into a hang rather than a crash. Worth a comment recording the counter-constraint, so the next person does not reorder this blind — and note the existing exchange-before-driver ordering has no such note either.
| sync->proceedEvent.notifyAll(); | ||
|
|
||
| // Correct order: drain httpSrvCpu before destroying driverExecutor. | ||
| httpSrvCpu->join(); |
There was a problem hiding this comment.
test does not pin the production ordering
Agreeing with Copilot's comment: this hardcodes httpSrvCpu->join(); driverExecutor.reset(); in the test body, so reverting the PrestoServer.cpp change leaves the test green.
On the "the other tests in this file do it the same way" reply — that is accurate, but weak justification here. Pinning the shutdown order is this file's only purpose, and with two opposing constraints now in play (see the comment on PrestoServer.cpp) nothing stops the next reorder from reintroducing the bug in either direction. Extracting the ordering into a helper that joinExecutors() calls, and driving that helper from the test, would cost little and would actually protect the fix.
| // Serialize on the CPU executor so the I/O thread only | ||
| // transmits pre-built bytes. | ||
| return util::dumpJson(json(result)); | ||
| return util::dumpJson(json(std::move(result))); |
There was a problem hiding this comment.
safe, but not applied to the sibling handler
The move is safe: result is the future's json::array_t&& and is not read afterwards.
The /v1/velox/plan handler below still does json(response) without the move (around line 2049), and response is equally dead after that point. Either apply the same change there or drop this hunk, so the two sidecar handlers stay consistent.
Minor: the PR description's "Better performance" claim rests entirely on this one line, not on the shutdown reordering.
|
I re-checked the current head ( I do not think the new test demonstrates the claimed By contrast, the test invents The narrow fix seems to be passing I also could not substantiate a new driver-to-HTTP late-enqueue regression from the |
Description
Ensure native server shutdown drains HTTP server CPU executor before joining the driver executor.
Motivation and Context
The
/v1/expressionsendpoint dispatches its work tohttpSrvCpuExecutor_, which passes a rawdriverExecutor_.get()pointer intoQueryCtx. Before this fix,joinExecutors()calleddriverExecutor_.reset()while those threads could still be running — a use-after-free.httpSrvCpuExecutor_->join()was originally placed near the end ofjoinExecutors(), well afterdriverExecutor_was already destroyed.Impact
Better performance and removes silent memory corruption.
Test Plan
Unit tests, CI
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Safely drain HTTP server CPU work before joining the driver executor during native server shutdown.
Bug Fixes:
Enhancements:
Tests: