You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/05 12:38:30 UTC
[GitHub] [nifi-minifi-cpp] dam4rus opened a new pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
dam4rus opened a new pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022
…ned true
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589223829
##########
File path: libminifi/test/unit/ThreadPoolTests.cpp
##########
@@ -70,8 +70,7 @@ TEST_CASE("ThreadPoolTest1", "[TPT1]") {
std::function<bool()> f_ex = function;
utils::Worker<bool> functor(f_ex, "id");
pool.start();
- std::future<bool> fut;
- REQUIRE(true == pool.execute(std::move(functor), fut));
+ auto fut = pool.execute(std::move(functor));
fut.wait();
Review comment:
should we assert the validity of the returned future in this test?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589249439
##########
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:
Who cleans up these futures allocated with `new`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589250748
##########
File path: libminifi/test/unit/ThreadPoolTests.cpp
##########
@@ -70,8 +70,7 @@ TEST_CASE("ThreadPoolTest1", "[TPT1]") {
std::function<bool()> f_ex = function;
utils::Worker<bool> functor(f_ex, "id");
pool.start();
- std::future<bool> fut;
- REQUIRE(true == pool.execute(std::move(functor), fut));
+ auto fut = pool.execute(std::move(functor));
fut.wait();
Review comment:
a default constructed future and a future that has been moved from are also `valid() == false`, my only problem is that operating on an "invalid" future is undefined behavior (although encouraged to throw), but I believe we can safely assume that `ThreadPool<T>::execute` returns a valid future, in which case I would remove the validity checks from `SchedulingAgent.cpp`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] dam4rus commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] dam4rus commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
dam4rus commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589245421
##########
File path: libminifi/test/unit/ThreadPoolTests.cpp
##########
@@ -70,8 +70,7 @@ TEST_CASE("ThreadPoolTest1", "[TPT1]") {
std::function<bool()> f_ex = function;
utils::Worker<bool> functor(f_ex, "id");
pool.start();
- std::future<bool> fut;
- REQUIRE(true == pool.execute(std::move(functor), fut));
+ auto fut = pool.execute(std::move(functor));
fut.wait();
Review comment:
I'm not sure about whether it makes sense to test for validity or not to be honest. Judging by the documentation https://en.cppreference.com/w/cpp/thread/future/valid `std::future::valid` only returns `false` if `std::future::get` has been called. I can revert the assertion but I assume it would be always `true`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r591540089
##########
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:
It's cleaned up by subsequent `onTrigger` calls, see the branch below. The list is also modified by `SocketAfterExecute`, which looks like it leaks on error, but I didn't verify.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] dam4rus commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
dam4rus commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589318656
##########
File path: libminifi/test/unit/ThreadPoolTests.cpp
##########
@@ -70,8 +70,7 @@ TEST_CASE("ThreadPoolTest1", "[TPT1]") {
std::function<bool()> f_ex = function;
utils::Worker<bool> functor(f_ex, "id");
pool.start();
- std::future<bool> fut;
- REQUIRE(true == pool.execute(std::move(functor), fut));
+ auto fut = pool.execute(std::move(functor));
fut.wait();
Review comment:
Sorry for the confusing comment. I meant that in this case `valid` would be only `false` after calling `get`/`share`. If I get this right, `std::promise::get_future` can't return an invalid `std::future` since it throws an exception when called on an invalid `std::promise`. E.g. it has no state or `get_future` has already been called once.
I'm not super familiar with `std::promise` to be honest but I think it's safe to remove validity checks in these cases. Wouldn't an invalid `std::future` would have cause problems by now?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589249439
##########
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:
Who cleans up/manages lifetime of these futures allocated with `new`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] dam4rus commented on a change in pull request #1022: Refactor ThreadPool::execute to return a future since it always retur…
Posted by GitBox <gi...@apache.org>.
dam4rus commented on a change in pull request #1022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1022#discussion_r589324834
##########
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:
The only place I've seen a `delete` on these future objects are here https://github.com/apache/nifi-minifi-cpp/pull/1022/files/a46b3bea392683bda836a9c668289c02810a371a#diff-8d484e99475c978743af82b7ee8ba45954457ecbac4c033744d1cc95ad9f8240R294
Since `GetTCP::live_clients_` is private I assume it's only deleted here. I can check it out to make sure but I think it's safe to rewrite `GetTCP::live_clients_` to store `std::unique_ptr`s or simply `std::future`s. AFAIK `std::future` is pretty cheap to move.
----------------------------------------------------------------
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:
users@infra.apache.org