szaszm commented on a change in pull request #1252:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1252#discussion_r809213842
##########
File path: libminifi/include/FlowController.h
##########
@@ -87,13 +91,8 @@ class FlowController : public
core::controller::ForwardingControllerServiceProvi
return this->provenance_repo_;
}
- // Get the flowfile repository
- virtual std::shared_ptr<core::Repository> getFlowFileRepository() {
- return this->flow_file_repo_;
- }
-
// Load flow xml from disk, after that, create the root process group and
its children, initialize the flows
- virtual void load(const std::shared_ptr<core::ProcessGroup> &root = nullptr,
bool reload = false);
+ virtual void load(std::unique_ptr<core::ProcessGroup>&& root = nullptr, bool
reload = false);
Review comment:
Pass the pointer by value instead of rvalue reference. Since it's
move-only, the usage will be the same, but the implementation will have to move
out from the parameter. It's not a big deal, though, we might even save some
time by moving twice, but without an extra level of indirection.
##########
File path: libminifi/include/core/state/nodes/QueueMetrics.h
##########
@@ -57,16 +57,16 @@ class QueueMetrics : public ResponseNode {
return "QueueMetrics";
}
- void addConnection(const std::shared_ptr<minifi::Connection> &connection) {
+ void addConnection(std::unique_ptr<minifi::Connection>&& connection) {
Review comment:
Consider pass-by-value for unique_ptr's. They are not too expensive to
move.
More details, if you're interested: https://youtu.be/rHIkrotSwcc?t=1050
##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -1034,14 +1030,18 @@ void ProcessSession::ensureNonNullResourceClaim(
}
std::shared_ptr<core::FlowFile> ProcessSession::get() {
- std::shared_ptr<Connectable> first =
process_context_->getProcessorNode()->pickIncomingConnection();
+ const auto first =
process_context_->getProcessorNode()->pickIncomingConnection();
if (first == nullptr) {
logger_->log_trace("Get is null for %s",
process_context_->getProcessorNode()->getName());
return nullptr;
}
- std::shared_ptr<Connection> current =
std::static_pointer_cast<Connection>(first);
+ auto current = dynamic_cast<Connection*>(first);
+ if (!current) {
+ logger_->log_error("Get could not find current Connection for %s",
process_context_->getProcessorNode()->getName());
Review comment:
I suggest a more detailed error message:
```suggestion
logger_->log_error("The incoming connection [%s] of the processor [%s]
\"%s\" is not actually a Connection.",
first->getUUIDStr(),
process_context_->getProcessorNode()->getUUIDStr(),
process_context_->getProcessorNode()->getName());
```
##########
File path: libminifi/src/core/ProcessGroup.cpp
##########
@@ -91,33 +91,16 @@ ProcessGroup::~ProcessGroup() {
}
}
-bool ProcessGroup::isRootProcessGroup() {
- return (type_ == ROOT_PROCESS_GROUP);
-}
-
bool ProcessGroup::isRemoteProcessGroup() {
return (type_ == REMOTE_PROCESS_GROUP);
}
-void ProcessGroup::addProcessor(const std::shared_ptr<Processor>& processor) {
+void ProcessGroup::addProcessor(std::unique_ptr<Processor>&& processor) {
Review comment:
Consider pass-by-value and a precondition to check that it's not null
##########
File path: libminifi/include/utils/GeneralUtils.h
##########
@@ -34,6 +34,16 @@ constexpr T intdiv_ceil(T numerator, T denominator) {
: numerator / denominator + (numerator % denominator != 0));
}
+/**
+ * safely converts unique_ptr from one type to another
+ * if conversion succeeds, an desired valid unique_ptr is returned, the "from"
object released and invalidated
+ * if conversion fails, an empty unique_ptr is returned
+ */
+template <typename T_To, typename T_From>
+std::unique_ptr<T_To> dynamic_unique_cast(std::unique_ptr<T_From>&& obj) {
Review comment:
I think it would be cleaner semantics to pass-by-value. This would mean
that we release the object regardless of the success of the cast, which is a
downside, but reading the client code will be simpler, because when we see
passing with `std::move`, we know that the old object WILL be null.
##########
File path: libminifi/src/core/Connectable.cpp
##########
@@ -141,39 +141,37 @@ void Connectable::notifyWork() {
}
}
-std::set<std::shared_ptr<Connectable>>
Connectable::getOutGoingConnections(const std::string &relationship) const {
- std::set<std::shared_ptr<Connectable>> empty;
-
- const auto &&it = out_going_connections_.find(relationship);
- if (it != out_going_connections_.end()) {
+std::set<Connectable*> Connectable::getOutGoingConnections(const std::string
&relationship) const {
+ const auto &&it = outgoing_connections_.find(relationship);
Review comment:
Would you mind removing the `&&`? This only works because of temporary
lifetime extension, but there is no reason why we wouldn't just store the
iterator by value like normal.
##########
File path: libminifi/include/processors/ProcessorUtils.h
##########
@@ -47,10 +45,10 @@ class ProcessorUtils {
if (ptr == nullptr) {
ptr =
core::ClassLoader::getDefaultClassLoader().instantiate("ExecuteJavaClass",
uuid);
if (ptr != nullptr) {
- std::shared_ptr<core::Processor> processor =
std::dynamic_pointer_cast<core::Processor>(ptr);
- if (processor == nullptr) {
+ if (dynamic_cast<core::Processor*>(ptr.get()) == nullptr) {
throw std::runtime_error("Invalid return from the classloader");
}
+ auto processor =
std::unique_ptr<core::Processor>{dynamic_cast<core::Processor*>(ptr.release())};
Review comment:
Use the dynamic unique cast utility here as well.
##########
File path: libminifi/include/processors/ProcessorUtils.h
##########
@@ -61,25 +59,15 @@ class ProcessorUtils {
if (ptr == nullptr) {
return nullptr;
}
- auto returnPtr = std::dynamic_pointer_cast<core::Processor>(ptr);
-
- if (returnPtr == nullptr) {
+ if (dynamic_cast<core::Processor*>(ptr.get()) == nullptr) {
throw std::runtime_error("Invalid return from the classloader");
}
+ auto returnPtr =
std::unique_ptr<core::Processor>{dynamic_cast<core::Processor*>(ptr.release())};
Review comment:
and here.
```suggestion
auto returnPtr =
utils::dynamic_unique_cast<core::Processor>(std::move(ptr));
if (!returnPtr) {
throw std::runtime_error("Invalid return from the classloader");
}
```
##########
File path: libminifi/src/FlowController.cpp
##########
@@ -271,7 +271,7 @@ std::unique_ptr<core::ProcessGroup>
FlowController::loadInitialFlow() {
return root;
}
-void FlowController::load(const std::shared_ptr<core::ProcessGroup> &root,
bool reload) {
+void FlowController::load(std::unique_ptr<core::ProcessGroup>&& root, bool
reload) {
Review comment:
Consider pass-by-value
##########
File path: libminifi/src/c2/C2Client.cpp
##########
@@ -115,7 +116,8 @@ void
C2Client::initialize(core::controller::ControllerServiceProvider *controlle
flowMonitor->setFlowVersion(flow_configuration_->getFlowVersion());
}
std::lock_guard<std::mutex> guard(metrics_mutex_);
- root_response_nodes_[response_node->getName()] = response_node;
+ std::unique_ptr<state::response::ResponseNode>
responseNodeToStore{dynamic_cast<state::response::ResponseNode*>(instance.release())};
Review comment:
use `utils::dynamic_unique_cast`
Whenever `release()` is called on a unique_ptr, it requires extra attention.
Just by looking around for `release()` in our codebase, I found a memory leak
(in the openwsman extension). I find this separation of the check (line 91) and
the release (here) to be very error-prone.
--
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]
