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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
