Skip to content

Mem leak - executor fix #3821

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.

Sign up for GitHub

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

Open
rasapala wants to merge 2 commits into
base: main
Choose a base branch
from
+4 −2
Open

Mem leak - executor fix #3821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d9f1bc
executor fix
rasapala Nov 26, 2025
3a96a88
Fix code
rasapala Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/drogon_http_async_writer_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ void DrogonHttpAsyncWriterImpl::PartialReplyBegin(std::function<void()> actualWo
this->responsePtr = drogon::HttpResponse::newAsyncStreamResponse(
[this, actualWorkloadCallback = std::move(actualWorkloadCallback)](drogon::ResponseStreamPtr stream) {
this->stream = std::move(stream);
this->pool.Schedule([actualWorkloadCallback = std::move(actualWorkloadCallback)] {
this->pool.Schedule([&actualWorkloadCallback] {
Copy link
Collaborator

@dkalinowski dkalinowski Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what ensures us that the lifetime of actualWorkloadCallback will be greater than Scheduler task lifetime?

Copy link
Collaborator

@atobiszei atobiszei Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsafe IMO -> we don't know when this code executes. Other part of fix looks fine.
https://github.com/google-ai-edge/mediapipe/blob/a5d8add41148e9eb18093c31e01e4bc8214080b3/mediapipe/framework/deps/threadpool_pthread_impl.cc#L152
https://github.com/google-ai-edge/mediapipe/blob/a5d8add41148e9eb18093c31e01e4bc8214080b3/mediapipe/framework/deps/threadpool_std_thread_impl.cc#L105

SPDLOG_DEBUG("DrogonHttpAsyncWriterImpl::PartialReplyBegin::Schedule begin");
try {
actualWorkloadCallback(); // run actual workload (mediapipe executor inferStream) which uses PartialReply
Expand Down
4 changes: 3 additions & 1 deletion src/http_rest_api_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpReque
serverReaderWriter->OverwriteResponseHeader("Content-Type", "text/event-stream");
serverReaderWriter->OverwriteResponseHeader("Cache-Control", "no-cache");
serverReaderWriter->OverwriteResponseHeader("Connection", "keep-alive");
serverReaderWriter->PartialReplyBegin([executor = std::move(executor), serverReaderWriter, request = std::move(request)] {
serverReaderWriter->PartialReplyBegin([executor = std::move(executor), serverReaderWriter, request = std::move(request)]() mutable {
ExecutionContext executionContext{ExecutionContext::Interface::REST, ExecutionContext::Method::V3Stream};
auto status = executor->inferStream(request, *serverReaderWriter, executionContext);
if (!status.ok()) {
Expand All @@ -729,6 +729,8 @@ Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpReque
serverReaderWriter->PartialReplyWithStatus(buffer.GetString(), HTTPStatusCode::BAD_REQUEST);
}
serverReaderWriter->PartialReplyEnd();
// Release executor resources after the work is done - for example CB pipeline object
executor.reset()
});
return StatusCode::PARTIAL_END;
}
Expand Down