Copilot commented on code in PR #2249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2249#discussion_r3861874949
##########
libminifi/test/unit/ProcessSessionTests.cpp:
##########
@@ -136,3 +138,31 @@ TEST_CASE("Test ProcessSession::write's possible
outcomes") {
ContentRepositoryDependentTests::testErrWrite(std::make_shared<core::repository::FileSystemRepository>());
ContentRepositoryDependentTests::testCancelWrite(std::make_shared<core::repository::FileSystemRepository>());
}
+
+TEST_CASE("Stable C API does not leak the flow file handle",
"[minifi-api][flowfilehandle]") {
+ Fixture fixture;
+ auto& process_session = fixture.processSession();
+ auto* c_session = minifi::utils::toC(&process_session);
+
+ auto created = process_session.create();
+ process_session.transfer(created, Success);
+ process_session.commit();
+
+ // Track the underlying FlowFile object; drop every strong reference we hold
locally.
+ std::weak_ptr<minifi::core::FlowFile> weak_flow_file = created;
+ created.reset();
+
+ // Pull the flow file out through the stable C API. This does `new
std::shared_ptr<FlowFile>`
+ // behind the returned handle, so the handle owns one strong reference to
the FlowFile.
+ auto* handle = minifi_process_session_get(c_session);
+ REQUIRE(handle != nullptr);
+
+ // Hand the (MINIFI_OWNED) handle back to minifi and drop the flow file for
good.
+ REQUIRE(minifi_process_session_remove(c_session, handle) ==
MINIFI_STATUS_SUCCESS);
+ process_session.commit();
+
+ // After remove+commit no legitimate owner remains. If the C API failed to
free the heap
+ // std::shared_ptr<FlowFile> behind the handle, that leaked strong reference
keeps the
+ // FlowFile alive and this assertion fails.
+ REQUIRE(weak_flow_file.expired());
+}
Review Comment:
This regression test is important for memcheck, but it’s only tagged via
Catch2 (`[minifi-api][flowfilehandle]`). In this PR you also add CTest `LABELS
memchecked` for some extension test binaries, which suggests the memcheck
workflow may select tests by CTest label. If memcheck CI filters on `-L
memchecked`, this test won’t be executed. Consider also labeling the
corresponding CTest entry for the `ProcessSessionTests` binary as `memchecked`
(or moving/duplicating the test into the stable-api-testing test suite that’s
already labeled).
##########
libminifi/src/minifi-api.cpp:
##########
@@ -456,8 +456,10 @@ minifi_status
minifi_process_session_penalize(minifi_process_session* session, m
minifi_status minifi_process_session_transfer(minifi_process_session* session,
MINIFI_OWNED minifi_flow_file* flowfile, minifi_string_view relationship_name) {
gsl_Assert(session);
gsl_Assert(flowfile);
+ // Adopt the MINIFI_OWNED handle: minifi takes ownership, so free the heap
std::shared_ptr<FlowFile> when this call returns.
+ const std::unique_ptr<std::shared_ptr<minifi::core::FlowFile>>
owned{toCpp(flowfile)};
try {
- toCpp(session)->transfer(*toCpp(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ toCpp(session)->transfer(*owned,
minifi::core::Relationship{toString(relationship_name), ""});
return MINIFI_STATUS_SUCCESS;
Review Comment:
This PR changes ownership semantics in `minifi_process_session_transfer`
(the handle is now always freed at function exit). There’s a regression test
for `remove`, but not for `transfer`. Please add a matching test that passes a
`MINIFI_OWNED` flowfile handle into `minifi_process_session_transfer(...)` and
asserts that the FlowFile does not remain alive solely due to the handle (i.e.,
the heap `std::shared_ptr<FlowFile>` is released).
##########
libminifi/src/minifi-api.cpp:
##########
@@ -467,8 +469,10 @@ minifi_status
minifi_process_session_transfer(minifi_process_session* session, M
minifi_status minifi_process_session_remove(minifi_process_session* session,
MINIFI_OWNED minifi_flow_file* flowfile) {
gsl_Assert(session);
gsl_Assert(flowfile);
+ // Adopt the MINIFI_OWNED handle: minifi takes ownership, so free the heap
std::shared_ptr<FlowFile> when this call returns.
+ const std::unique_ptr<std::shared_ptr<minifi::core::FlowFile>>
owned{toCpp(flowfile)};
Review Comment:
Declaring the `std::unique_ptr` as `const` is unusual and slightly obscures
intent (the key property is scoped ownership and destruction). Making it
non-const (e.g., `auto owned = ...`) improves readability and avoids friction
if the function later needs to `reset()` or conditionally release ownership.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
