You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "fgerlits (via GitHub)" <gi...@apache.org> on 2026/09/03 14:46:34 UTC
[PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
fgerlits opened a new pull request, #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254
ASan/LSan fixes:
* fix a SEGV-at-shutdown error in Tcp/UdpServer
* fix a use-after-free bug in TestUtils
* fix a leak in the libssh2 library
* free the shared_ptr result of async_resolve when it is no longer needed
* fix a leak in HTTPSiteToSiteTest
* change a backward link to raw pointer to break a shared_ptr cycle
* fix a leak in the Python bindings
* fix an alloc-dealloc-mismatch bug: new + free (test code only)
ODR violation warnings are suppressed, because the way we link and dlopen libraries causes many false positives. If two symbols are defined with the same name but different sizes, they will still be flagged.
Upgrade Abseil to the latest release to fix a leak, and upgrade Grpc to the latest release because the old Grpc did not like the new Abseil.
Add a new ASan/LSan job to the weekly memcheck CI job.
Change `CMAKE_C_FLAGS` to `PASSTHROUGH_CMAKE_C_FLAGS` in cmake/BundledIodbc.cmake -- this is unrelated to ASan, but it was clearly a bug, and it's a one-line fix. I will remove it from this PR if requested.
---
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3990020235
##########
libminifi/test/unit/FileStreamTests.cpp:
##########
@@ -124,36 +124,6 @@ TEST_CASE("TestFileBadArgumentNoChange2", "[TestLoader]") {
REQUIRE(std::string(reinterpret_cast<char*>(data), verifybuffer.size()) == "tempFile");
}
-TEST_CASE("TestFileBadArgumentNoChange3", "[TestLoader]") {
- TestController testController;
- auto path = testController.createTempDirectory() / "tstFile.ext";
Review Comment:
I removed this test because now it is basically identical to the previous one, and gcc 15.2 did not like the `string` constructor at line 154. This test case used to test `stream.read(nullptr, 0)`, but we no longer support this pointer syntax, only `std::span`s.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3956697739
##########
cmake/BuildTests.cmake:
##########
@@ -17,6 +17,22 @@
include(GetCatch2)
+if (MINIFI_ADVANCED_ASAN_BUILD)
+ file(MAKE_DIRECTORY "${CMAKE_BINARY_DIR}/asan_logs")
+
+ # Route each test's AddressSanitizer/LeakSanitizer output to <build>/asan_logs/<test-name>.<pid>
+ # and suppress odr-violation warnings (only if the two symbols have the same size). Hundreds of global
+ # symbols are present in two or more .so's, so we don't want to suppress each individually.
+ function(add_test)
+ _add_test(${ARGV})
+ cmake_parse_arguments(MINIFI_TEST "" "NAME" "COMMAND" ${ARGV})
+ if (MINIFI_TEST_NAME)
+ set_property(TEST "${MINIFI_TEST_NAME}" APPEND PROPERTY
+ ENVIRONMENT "ASAN_OPTIONS=detect_odr_violation=1:log_path=${CMAKE_BINARY_DIR}/asan_logs/${MINIFI_TEST_NAME}")
+ endif()
+ endfunction()
Review Comment:
`_add_test(${ARGV})` is not a built-in CMake command; unless `_add_test` is defined elsewhere before this block executes, this will fail configuration when `MINIFI_ADVANCED_ASAN_BUILD` is ON. Prefer calling the original command via `cmake_language(CALL COMMAND add_test ...)`, or alias the original `add_test` to `_add_test` before overriding it.
##########
libminifi/include/core/controller/StandardControllerServiceNode.h:
##########
@@ -31,10 +31,9 @@ namespace org::apache::nifi::minifi::core::controller {
class StandardControllerServiceNode : public ControllerServiceNode {
public:
- explicit StandardControllerServiceNode(std::shared_ptr<ControllerService> service, std::shared_ptr<ControllerServiceProvider> provider, std::string id,
- std::shared_ptr<Configure> configuration)
+ explicit StandardControllerServiceNode(std::shared_ptr<ControllerService> service, ControllerServiceProvider* provider, std::string id, std::shared_ptr<Configure> configuration)
: ControllerServiceNode(std::move(service), std::move(id), std::move(configuration)),
- provider(std::move(provider)),
+ provider(provider),
Review Comment:
Switching `provider` from `std::shared_ptr` to a raw pointer removes lifetime safety. If `StandardControllerServiceNode` can ever outlive its provider, this becomes a potential dangling pointer. To make the contract explicit, consider storing a reference (`ControllerServiceProvider&`) or a non-null wrapper (e.g., `gsl::not_null<ControllerServiceProvider*>`) and documenting the required lifetime relationship.
##########
extension-framework/include/utils/net/Server.h:
##########
@@ -67,9 +75,21 @@ class Server {
Server(std::optional<size_t> max_queue_size, uint16_t port, std::shared_ptr<core::logging::Logger> logger)
: port_(port), max_queue_size_(max_queue_size), logger_(std::move(logger)) {}
+ // Spawn a coroutine on io_context_ with a cancellation slot so stop() can end it and let the context drain
+ // gracefully. Must be called from the io_context thread (i.e. from run() before io_context_.run(), or from within
+ // a coroutine running on it); cancellation_signals_ is only ever touched on that thread, so it needs no locking.
+ template<typename T>
+ void asyncSpawn(asio::awaitable<T> coroutine) {
+ const auto cancellation_signal_it = cancellation_signals_.emplace(cancellation_signals_.end());
+ asio::co_spawn(io_context_, std::move(coroutine),
+ asio::bind_cancellation_slot(cancellation_signal_it->slot(),
+ [this, cancellation_signal_it](std::exception_ptr, auto&&...) { cancellation_signals_.erase(cancellation_signal_it); }));
+ }
Review Comment:
The completion handler passed to `co_spawn` ignores the `std::exception_ptr`, which can silently swallow exceptions thrown from spawned coroutines (whereas `asio::detached` typically terminates on unhandled exceptions). Consider preserving the previous failure semantics by explicitly handling `exception_ptr` (e.g., log + terminate, or rethrow) before erasing the cancellation signal.
##########
libminifi/include/core/controller/StandardControllerServiceNode.h:
##########
@@ -57,7 +56,7 @@ class StandardControllerServiceNode : public ControllerServiceNode {
bool disable() override;
protected:
- std::shared_ptr<ControllerServiceProvider> provider;
+ ControllerServiceProvider* provider;
Review Comment:
Switching `provider` from `std::shared_ptr` to a raw pointer removes lifetime safety. If `StandardControllerServiceNode` can ever outlive its provider, this becomes a potential dangling pointer. To make the contract explicit, consider storing a reference (`ControllerServiceProvider&`) or a non-null wrapper (e.g., `gsl::not_null<ControllerServiceProvider*>`) and documenting the required lifetime relationship.
##########
extensions/python/types/Types.h:
##########
@@ -246,7 +246,8 @@ class List : public ReferenceHolder<reference_type> {
template<object::convertible T>
void append(T value) {
- PyList_Append(this->ref_.get(), object::from(std::move(value)).releaseReference());
+ auto object = object::from(std::move(value));
+ PyList_Append(this->ref_.get(), object.get());
Review Comment:
The local variable name `object` is easily confused with (and shadows) the `object` namespace used in the initializer. Rename the local (e.g., `py_object`, `item`, `value_obj`) to keep the code unambiguous.
##########
.github/workflows/memcheck_ci.yml:
##########
@@ -75,3 +75,50 @@ jobs:
build/Testing/Temporary/MemoryChecker.*.log
build/Testing/Temporary/LastDynamicAnalysis_*.log
if-no-files-found: ignore
+ address-sanitizer:
+ name: "AdressSanitizer+LeakSanitizer on ubuntu 26.04"
Review Comment:
Typo in the job display name: 'AdressSanitizer' should be 'AddressSanitizer'.
##########
extensions/python/types/Types.h:
##########
@@ -296,7 +297,8 @@ class Dict : public ReferenceHolder<reference_type> {
template<object::convertible T>
void put(const char* key, T value) {
- PyDict_SetItemString(this->ref_.get(), key, object::from(std::move(value)).releaseReference());
+ auto object = object::from(std::move(value));
+ PyDict_SetItemString(this->ref_.get(), key, object.get());
Review Comment:
Same as in `List::append`: using `object` as a local variable name is ambiguous next to the `object::` namespace qualifier. Rename the local variable to improve clarity and avoid namespace/variable shadowing confusion.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3989888514
##########
cmake/BundledIodbc.cmake:
##########
@@ -33,7 +33,7 @@ ExternalProject_Add(
CMAKE_COMMAND ""
UPDATE_COMMAND ""
INSTALL_COMMAND make install
- CONFIGURE_COMMAND bash "-c" "./autogen.sh && ./configure --prefix=${IODBC_BYPRODUCT_DIR} --with-pic CFLAGS='${CMAKE_C_FLAGS} -std=gnu17'"
+ CONFIGURE_COMMAND bash "-c" "./autogen.sh && ./configure --prefix=${IODBC_BYPRODUCT_DIR} --with-pic CFLAGS='${PASSTHROUGH_CMAKE_C_FLAGS} -std=gnu17'"
Review Comment:
This is unrelated to ASan, but it was clearly a bug, and it's a one-line fix. I will remove it from this PR if requested.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4024368256
##########
extension-framework/include/utils/net/ConnectionHandler.h:
##########
@@ -64,7 +66,7 @@ class ConnectionHandler final : public ConnectionHandlerBase {
[[nodiscard]] asio::awaitable<std::error_code> setupUsableSocket(asio::io_context& io_context) override;
[[nodiscard]] bool hasUsableSocket() const { return socket_ && socket_->lowest_layer().is_open(); }
- asio::awaitable<std::error_code> establishNewConnection(const asio::ip::tcp::resolver::results_type& endpoints, asio::io_context& io_context_);
+ asio::awaitable<std::error_code> establishNewConnection(const std::vector<asio::ip::tcp::endpoint>& endpoints, asio::io_context& io_context_);
Review Comment:
There was a leak in `ConnectionHandler<SocketType>::setupUsableSocket` (lines 147-163 in the same file) because the `resolve_result` did not get deleted properly. I don't completely understand why this happened(*), but the fix was to copy the endpoints from the `resolve_result` and let it go out of scope before the next `co_await` is called.
Using `span` instead `vector` as the argument type is a good idea, done in 0dcbf880b53a9921b5bbd43f73c2e4240f816130.
(*) it may be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124584, as I have checked that it happens on GCC 15, 16, but doesn't happen on GCC 13, 14 and Clang 20.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm merged PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#issuecomment-5697221941
> could we now perhaps expand the memchecked tag or maybe even remove the tag and run everything not just memchecked tagged in the valgrind ci?
>
> otherwise lgtm 👍
Good idea, but I'd prefer to put that in a separate PR, since it's not related to ASan, and it's likely to find a lot of new issues.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4024368256
##########
extension-framework/include/utils/net/ConnectionHandler.h:
##########
@@ -64,7 +66,7 @@ class ConnectionHandler final : public ConnectionHandlerBase {
[[nodiscard]] asio::awaitable<std::error_code> setupUsableSocket(asio::io_context& io_context) override;
[[nodiscard]] bool hasUsableSocket() const { return socket_ && socket_->lowest_layer().is_open(); }
- asio::awaitable<std::error_code> establishNewConnection(const asio::ip::tcp::resolver::results_type& endpoints, asio::io_context& io_context_);
+ asio::awaitable<std::error_code> establishNewConnection(const std::vector<asio::ip::tcp::endpoint>& endpoints, asio::io_context& io_context_);
Review Comment:
There was a leak in `ConnectionHandler<SocketType>::setupUsableSocket` (lines 147-163 in the same file) because the `resolve_result` did not get deleted properly. I don't completely understand why this happened(*), but the fix was to copy the endpoints from the `resolve_result` and let it go out of scope before the next `co_await` is called.
Using `span` instead of `vector` as the argument type is a good idea, done in 0dcbf880b53a9921b5bbd43f73c2e4240f816130.
(*) it may be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124584, as I have checked that it happens on GCC 15, 16, but doesn't happen on GCC 13, 14 and Clang 20.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4044651645
##########
cmake/Abseil.cmake:
##########
@@ -28,8 +28,8 @@ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
FetchContent_Declare(
absl
- URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260526.0.tar.gz
- URL_HASH SHA256=6e1aee535473414164bf83e4ebc40240dec71a4701f8a642d906e95bea1aea0c
+ URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260817.0.tar.gz
Review Comment:
There was a leak in the 20260526 version which is fixed in the 20260817 version, so I upgraded instead of patching.
We should review and upgrade both Conan and non-Conan dependency versions periodically. Maybe after each release, so there is time to find and fix any issues before the next release?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3970525270
##########
cmake/BuildTests.cmake:
##########
@@ -17,6 +17,22 @@
include(GetCatch2)
+if (MINIFI_ADVANCED_ASAN_BUILD)
+ file(MAKE_DIRECTORY "${CMAKE_BINARY_DIR}/asan_logs")
+
+ # Route each test's AddressSanitizer/LeakSanitizer output to <build>/asan_logs/<test-name>.<pid>
+ # and suppress odr-violation warnings (only if the two symbols have the same size). Hundreds of global
+ # symbols are present in two or more .so's, so we don't want to suppress each individually.
+ function(add_test)
+ _add_test(${ARGV})
+ cmake_parse_arguments(MINIFI_TEST "" "NAME" "COMMAND" ${ARGV})
+ if (MINIFI_TEST_NAME)
+ set_property(TEST "${MINIFI_TEST_NAME}" APPEND PROPERTY
+ ENVIRONMENT "ASAN_OPTIONS=detect_odr_violation=1:log_path=${CMAKE_BINARY_DIR}/asan_logs/${MINIFI_TEST_NAME}")
+ endif()
+ endfunction()
Review Comment:
If you redefine a built-in CMake function, the original function will be available inside the new implementation with an underscore prepended, so this does work. This is a widely used CMake technique (so CMake is unlikely to drop it, since that would break many projects), but it is not officially supported, and has some issues (e.g., redefining the same function a second time will not work).
I don't think Copilot's solutions would work without creating an infinite call loop. We could write a `minifi_add_test` wrapper which adds the ASAN options when needed, but then all `add_test` calls would need to be changed to `minifi_add_test` and all future test writers will need to remember to use `minifi_add_test` instead of `add_test`. Or we could define the ASAN_OPTIONS globally, but then ASan results would be written to files named `asan.<pid>` (without the name of the test), which is not great.
I still think redefining `add_test` as above is the cleanest way to do this, but I will keep this comment open so others can comment.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4044779321
##########
cmake/Abseil.cmake:
##########
@@ -28,8 +28,8 @@ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
FetchContent_Declare(
absl
- URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260526.0.tar.gz
- URL_HASH SHA256=6e1aee535473414164bf83e4ebc40240dec71a4701f8a642d906e95bea1aea0c
+ URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260817.0.tar.gz
Review Comment:
In the age of AI security scanning, and lots of CVEs in each release, I think I'd time the updates closer to before a release, to minimize the number of known unpatched vulnerabilities in our libs.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3989941707
##########
libminifi/test/libtest/integration/HTTPHandlers.h:
##########
@@ -40,9 +40,6 @@
namespace org::apache::nifi::minifi::test {
-static std::atomic<int> transaction_id;
-static std::atomic<int> transaction_id_output;
Review Comment:
This is also unrelated to ASan, but we used a local variable named `transaction_id` somewhere where this header was included, and clang-tidy was complaining about hiding a global variable. So I moved these two to the .cpp file.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4037601903
##########
cmake/Abseil.cmake:
##########
@@ -28,8 +28,8 @@ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
FetchContent_Declare(
absl
- URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260526.0.tar.gz
- URL_HASH SHA256=6e1aee535473414164bf83e4ebc40240dec71a4701f8a642d906e95bea1aea0c
+ URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260817.0.tar.gz
Review Comment:
Did the older version have an issue that needed the update or was Abseil updated for another reason? I see that the Conan package version was not updated as Conan Center's version is not updated yet, so there will be a difference between the dependency versions of the package built from source and downloaded from Conan. I don't think that's a problem now, but we should make a not to update that one too when 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4037601903
##########
cmake/Abseil.cmake:
##########
@@ -28,8 +28,8 @@ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
FetchContent_Declare(
absl
- URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260526.0.tar.gz
- URL_HASH SHA256=6e1aee535473414164bf83e4ebc40240dec71a4701f8a642d906e95bea1aea0c
+ URL https://github.com/abseil/abseil-cpp/archive/refs/tags/20260817.0.tar.gz
Review Comment:
Did the older version have an issue that needed the update or was Abseil updated for another reason? I see that the Conan package version was not updated as Conan Center's version is not updated yet, so there will be a difference between the dependency versions of the package built from source and the one downloaded from Conan. I don't think that's a problem now, but we should make a note to update that one too when 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3970297181
##########
extension-framework/include/utils/net/Server.h:
##########
@@ -67,9 +75,21 @@ class Server {
Server(std::optional<size_t> max_queue_size, uint16_t port, std::shared_ptr<core::logging::Logger> logger)
: port_(port), max_queue_size_(max_queue_size), logger_(std::move(logger)) {}
+ // Spawn a coroutine on io_context_ with a cancellation slot so stop() can end it and let the context drain
+ // gracefully. Must be called from the io_context thread (i.e. from run() before io_context_.run(), or from within
+ // a coroutine running on it); cancellation_signals_ is only ever touched on that thread, so it needs no locking.
+ template<typename T>
+ void asyncSpawn(asio::awaitable<T> coroutine) {
+ const auto cancellation_signal_it = cancellation_signals_.emplace(cancellation_signals_.end());
+ asio::co_spawn(io_context_, std::move(coroutine),
+ asio::bind_cancellation_slot(cancellation_signal_it->slot(),
+ [this, cancellation_signal_it](std::exception_ptr, auto&&...) { cancellation_signals_.erase(cancellation_signal_it); }));
+ }
Review Comment:
Exceptions were also swallowed silently before this PR; I have added a comment about this.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r3970320695
##########
libminifi/include/core/controller/StandardControllerServiceNode.h:
##########
@@ -31,10 +31,9 @@ namespace org::apache::nifi::minifi::core::controller {
class StandardControllerServiceNode : public ControllerServiceNode {
public:
- explicit StandardControllerServiceNode(std::shared_ptr<ControllerService> service, std::shared_ptr<ControllerServiceProvider> provider, std::string id,
- std::shared_ptr<Configure> configuration)
+ explicit StandardControllerServiceNode(std::shared_ptr<ControllerService> service, ControllerServiceProvider* provider, std::string id, std::shared_ptr<Configure> configuration)
: ControllerServiceNode(std::move(service), std::move(id), std::move(configuration)),
- provider(std::move(provider)),
+ provider(provider),
Review Comment:
Good point. I will revert this change, for now, as fixing it would be a bigger change, and should go in a separate pull request.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#issuecomment-5693491555
successful run of the new memcheck job: https://github.com/fgerlits/nifi-minifi-cpp/actions/runs/35003944750
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2877 Fix ASan and LSan warnings [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2254:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2254#discussion_r4017892607
##########
extension-framework/include/utils/net/ConnectionHandler.h:
##########
@@ -64,7 +66,7 @@ class ConnectionHandler final : public ConnectionHandlerBase {
[[nodiscard]] asio::awaitable<std::error_code> setupUsableSocket(asio::io_context& io_context) override;
[[nodiscard]] bool hasUsableSocket() const { return socket_ && socket_->lowest_layer().is_open(); }
- asio::awaitable<std::error_code> establishNewConnection(const asio::ip::tcp::resolver::results_type& endpoints, asio::io_context& io_context_);
+ asio::awaitable<std::error_code> establishNewConnection(const std::vector<asio::ip::tcp::endpoint>& endpoints, asio::io_context& io_context_);
Review Comment:
Why change this type? If we change it, why not go with something more generic, like std::span?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org