Copilot commented on code in PR #2088:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2088#discussion_r2705131346
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -1140,24 +1158,31 @@ void C2Agent::handleAssetUpdate(const
C2ContentResponse& resp) {
return;
}
- C2Payload file_response = protocol_->fetch(url);
-
- if (file_response.getStatus().getState() !=
state::UpdateState::READ_COMPLETE) {
- send_error("Failed to fetch asset from '" + url + "'");
- return;
- }
-
- auto raw_data = std::move(file_response).moveRawData();
// ensure directory exists for file
if (utils::file::create_dir(file_path.parent_path()) != 0) {
send_error("Failed to create directory '" +
file_path.parent_path().string() + "'");
return;
}
- {
- std::ofstream file{file_path, std::ofstream::binary};
- file.write(reinterpret_cast<const char*>(raw_data.data()),
gsl::narrow<std::streamsize>(raw_data.size()));
+ std::filesystem::path tmp_file{file_path.string() + ".part"};
+
+ std::ofstream file{tmp_file, std::ofstream::binary};
+ if (!file) {
+ send_error("Failed to open asset file to write '" + tmp_file.string() +
"'");
+ return;
}
+ bool success = protocol_->fetch(url, [&] (std::span<const char> chunk) {
+ file.write(chunk.data(), gsl::narrow<std::streamsize>(chunk.size()));
+ return file.good();
+ });
+ file.close();
+ if (!file || !success) {
+ std::filesystem::remove(tmp_file);
+ send_error("Failed to fetch asset from '" + url + "'");
+ return;
+ }
+
+ std::filesystem::rename(tmp_file, file_path);
Review Comment:
The std::filesystem::rename function can throw an exception on error. This
call should be wrapped in error handling (try-catch or use the error_code
overload) to prevent unhandled exceptions from propagating and ensure proper
error reporting to the user.
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -1012,9 +1021,18 @@ std::optional<std::string> C2Agent::fetchFlow(const
std::string& uri) const {
return std::nullopt;
}
- C2Payload response = protocol_->fetch(resolved_url.value(),
update_sink_->getSupportedConfigurationFormats());
+ std::string flow_content;
+ bool success = protocol_->fetch(resolved_url.value(),
update_sink_->getSupportedConfigurationFormats(), [&] (std::span<const char>
chunk) {
+ flow_content.append(chunk.data(), chunk.size());
+ return true;
Review Comment:
The flow_content string is accumulated in memory as chunks arrive. For large
flows, this could consume significant memory. Consider if there's a maximum
expected flow size, or if this should stream to a file instead (similar to how
asset downloads are now handled).
##########
core-framework/include/http/HTTPStream.h:
##########
@@ -115,9 +115,9 @@ class HttpStream : public io::BaseStreamImpl {
inline bool isFinished(int seconds = 0) {
return http_client_future_.wait_for(std::chrono::seconds(seconds)) ==
std::future_status::ready
- && http_client_->getReadCallback()
- && http_client_->getReadCallback()->getSize() == 0
- && http_client_->getReadCallback()->waitingOps();
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())->getSize()
== 0
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())->waitingOps();
Review Comment:
The dynamic_cast is performed three times in this expression without caching
the result. This is inefficient and could be optimized by storing the cast
result in a local variable.
##########
libminifi/src/utils/file/AssetManager.cpp:
##########
@@ -156,9 +156,9 @@ nonstd::expected<void, std::string> AssetManager::sync(
}
}
- for (auto& [path, content] : new_file_contents) {
- create_dir((root_ / path).parent_path());
- std::ofstream{root_ / path, std::ios::binary}.write(reinterpret_cast<const
char*>(content.data()), gsl::narrow<std::streamsize>(content.size()));
+ for (auto& path : new_file_paths) {
+ create_dir(path.parent_path());
+ std::filesystem::rename(path.string() + ".part", path);
Review Comment:
The std::filesystem::rename function can throw an exception on error. This
call should be wrapped in error handling (try-catch or use the error_code
overload) to prevent unhandled exceptions from propagating, especially since
this is called in a loop and could fail for some files but not others.
##########
core-framework/include/http/HTTPStream.h:
##########
@@ -127,11 +127,11 @@ class HttpStream : public io::BaseStreamImpl {
do {
logger_->log_trace("Waiting for more data");
} while (http_client_future_.wait_for(std::chrono::seconds(0)) !=
std::future_status::ready
- && http_client_->getReadCallback()
- && http_client_->getReadCallback()->getSize() == 0);
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())->getSize()
== 0);
- return http_client_->getReadCallback()
- && http_client_->getReadCallback()->getSize() > 0;
+ return
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())->getSize()
> 0;
Review Comment:
The dynamic_cast is performed twice in this expression without caching the
result. This is inefficient and could be optimized by storing the cast result
in a local variable.
##########
libminifi/src/c2/protocols/RESTSender.cpp:
##########
@@ -76,6 +76,7 @@ void
RESTSender::initialize(core::controller::ControllerServiceProvider* control
req_encoding_ = RequestEncoding::none;
}
}
+ asset_download_timeout_ =
(configure->get(Configuration::nifi_c2_asset_download_timeout) |
utils::andThen([] (const auto& s) { return parsing::parseDuration(s) |
utils::toOptional(); })).value_or(0s);
Review Comment:
The asset_download_timeout_ initialization occurs outside the configure null
check block, which could lead to a null pointer dereference if configure is
nullptr. This line should be moved inside the if (nullptr != configure) block
at line 45 to prevent potential crashes.
```suggestion
asset_download_timeout_ =
(configure->get(Configuration::nifi_c2_asset_download_timeout) |
utils::andThen([] (const auto& s) { return parsing::parseDuration(s) |
utils::toOptional(); })).value_or(0s);
} else {
asset_download_timeout_ = 0s;
}
```
##########
core-framework/include/http/HTTPStream.h:
##########
@@ -127,11 +127,11 @@ class HttpStream : public io::BaseStreamImpl {
do {
logger_->log_trace("Waiting for more data");
} while (http_client_future_.wait_for(std::chrono::seconds(0)) !=
std::future_status::ready
- && http_client_->getReadCallback()
- && http_client_->getReadCallback()->getSize() == 0);
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())
+ &&
dynamic_cast<utils::ByteOutputCallback*>(http_client_->getReadCallback())->getSize()
== 0);
Review Comment:
The dynamic_cast is performed twice in this expression without caching the
result. This is inefficient and could be optimized by storing the cast result
in a local variable.
--
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]
