dam4rus commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r602870835
##########
File path: extensions/standard-processors/processors/GetTCP.cpp
##########
@@ -285,21 +285,17 @@ void GetTCP::onTrigger(const
std::shared_ptr<core::ProcessContext> &context, con
} else {
logger_->log_error("Could not create socket for %s", endpoint);
}
- auto* future = new std::future<int>();
std::unique_ptr<utils::AfterExecute<int>> after_execute =
std::unique_ptr<utils::AfterExecute<int>>(new SocketAfterExecute(running_,
endpoint, &live_clients_, &mutex_));
utils::Worker<int> functor(f_ex, "workers", std::move(after_execute));
- if (client_thread_pool_.execute(std::move(functor), *future)) {
- live_clients_[endpoint] = future;
- }
+ auto* future = new
std::future<int>(client_thread_pool_.execute(std::move(functor)));
Review comment:
Looking at the code it looks like `SocketAfterExecute::isFinished` is
indeed leaking the future. I think simply storing an `std::future` in the
`GetTCP::live_clients_` should clear this up.
The whole `SocketAfterExecute` class is kinda suspicious. It stores a copy
of `GetTCP::running_` while storing a pointer to `GetTCP::live_clients_` and
`GetTCP::mutex_`. Isn't it supposed to take a reference to `GetTCP::running_`
as well?
What are the lifetime relation of these objects? Can `SocketAfterExecute`
outlive `GetTCP`? If so storing an `std::weak_ptr` would be safer than raw
pointers. Otherwise references should would work as well but harder to reason
about. Maybe store `GetTCP::running_`, `GetTCP::live_clients_` and
`GetTCP::mutex_` in a struct and create a `std::shared_ptr` to it?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
