Baf 1155/use async client #47
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| * | ||
| * @param path path to the library | ||
| */ | ||
| virtual void loadLibrary(const std::filesystem::path &path) = 0; |
There was a problem hiding this comment.
Why for some functions the documentation is missing?
|
|
||
| namespace bringauto::modules { | ||
|
|
||
| struct ConvertibleBufferReturn final { |
There was a problem hiding this comment.
Each struct shall have its own file.
| } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition getModuleNumberAsync { |
There was a problem hiding this comment.
Function definitions shall be part of Fleet Protocol CPP - https://github.com/bringauto/fleet-protocol-cpp
(I guess not in the cxx namespace but in its own namespace?)
| current_command_raw_buffer = current_command.getStructBuffer(); | ||
| } | ||
|
|
||
| auto ret = aeronClient.callFunc(generateCommandAsync, |
There was a problem hiding this comment.
Why ret is not checked if it contains value? What if the expected returns "error"? Check it in all occurrences please.
source/bringauto/modules/memory_management/source/memory_management.cpp
Outdated
Show resolved
Hide resolved
| @@ -1,5 +1,7 @@ | |||
| #include <ErrorAggregatorTests.hpp> | |||
| #include <testing_utils/DeviceIdentificationHelper.h> | |||
| #include <bringauto/modules/ModuleManagerLibraryHandlerLocal.hpp> | |||
There was a problem hiding this comment.
I cannot find any related tests. Can we add unit tests for updates and verify that the updates do not "break" tests in a manner "It works, but it shall not"?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
include/bringauto/modules/IModuleManagerLibraryHandler.hpp (1)
30-30: Add documentation for undocumented interface methods.Several public virtual methods lack documentation comments:
getModuleNumber,isDeviceTypeSupported,sendStatusCondition,statusDataValid, andcommandDataValid. Since this is a new public interface, all methods should be documented to guide implementers and users.Also applies to: 32-32, 34-34, 68-68, 70-70
resources/config/README.md (1)
25-26: Expand documentation to fully explain the two module loading modes.The current documentation provides minimal information about
module-binary-path. The previous review comment requested a more comprehensive explanation of what a ModuleBinary is and that there are now two ways of using modules in Module Gateway. Consider expanding this section to:
- Explain the difference between loading modules as shared libraries (local/synchronous) vs. using module binaries (async over shared memory)
- Describe when to use each approach
- Clarify the implications of leaving
module-binary-pathempty vs. providing a pathinclude/bringauto/settings/Settings.hpp (1)
36-39: Consider usingstd::filesystem::pathfor the path field.A previous review comment suggested using
std::filesystem::pathinstead ofstd::stringfor path fields. The newmoduleBinaryPathfield usesstd::string, which is inconsistent with that recommendation. Usingstd::filesystem::pathprovides better type safety and path manipulation capabilities.Note: This also applies to the existing
modulePathsfield, which could be addressed in a separate refactoring effort.include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp (3)
15-51: Move structs to separate header files.As noted in a previous review comment,
ConvertibleBufferReturnandConvertibleBuffershould each be in their own header files for better organization and maintainability.Create:
include/bringauto/modules/ConvertibleBuffer.hppinclude/bringauto/modules/ConvertibleBufferReturn.hpp
53-105: Consider moving function definitions to fleet-protocol-cpp.As mentioned in a previous review comment, the async function definitions should be part of the Fleet Protocol CPP library rather than being defined here, possibly in a dedicated namespace.
194-194: Address the TODO comment about tmpMutex_.The TODO comment indicates incomplete design regarding thread safety. As noted in the implementation file review, clarify the thread safety requirements and either protect all concurrent accesses or document why selective protection is sufficient.
source/bringauto/modules/ModuleManagerLibraryHandlerAsync.cpp (4)
12-28: Make aeronClient a class member or static member.The global
aeronClientintroduces potential issues with lifetime management and makes it impossible to have multiple async handler instances with different configurations. This also relates to a previous review comment requesting it be part of the class.Consider making it a static member of the class:
+// In header file +class ModuleManagerLibraryHandlerAsync : public IModuleManagerLibraryHandler { +private: + static async_function_execution::AsyncFunctionExecutor& getAeronClient(); + // ... rest of class +}; +// In implementation file -async_function_execution::AsyncFunctionExecutor aeronClient { +async_function_execution::AsyncFunctionExecutor& ModuleManagerLibraryHandlerAsync::getAeronClient() { + static async_function_execution::AsyncFunctionExecutor client { - async_function_execution::Config { + async_function_execution::Config { - .isProducer = true, + .isProducer = true, - .defaultTimeout = std::chrono::seconds(1) + .defaultTimeout = std::chrono::seconds(1) - }, + }, - async_function_execution::FunctionList { + async_function_execution::FunctionList { - getModuleNumberAsync, + getModuleNumberAsync, // ... rest of functions - } -}; + } + }; + return client; +}
15-15: Replace hardcoded timeout with a named constant.The 1-second timeout is hardcoded. This should be a named constant, preferably defined in
Constants.hppalongside other timeout values, for better maintainability.In
include/bringauto/settings/Constants.hpp:constexpr std::chrono::seconds aeron_async_call_timeout { 1 };Then use it in the configuration:
- .defaultTimeout = std::chrono::seconds(1) + .defaultTimeout = settings::aeron_async_call_timeout
54-54: Check return values before calling .value().Multiple calls to
aeronClient.callFunc(...).value()assume the optional contains a value. If an error occurs in the async call,.value()will throwstd::bad_optional_access. This relates to a previous review comment requesting return value validation.Apply defensive checks throughout:
- return aeronClient.callFunc(getModuleNumberAsync).value(); + auto result = aeronClient.callFunc(getModuleNumberAsync); + if (!result.has_value()) { + throw std::runtime_error("Failed to get module number from async call"); + } + return result.value();Apply similar pattern to all
.value()calls in lines 54, 59, 75, 100, 123, 147, 157, 172, and 181.Also applies to: 59-59, 75-75, 100-100, 123-123, 147-147, 157-157, 172-172, 181-181
58-58: Incomplete thread safety with tmpMutex_.The mutex is only used in
isDeviceTypeSupported, but other methods likegenerateCommand,aggregateStatus, etc., also callaeronClient.callFunc()and may be invoked concurrently. This relates to a previous review comment about thread safety. Either protect all calls or document why selective protection is sufficient.Review the thread safety requirements for all methods:
#!/bin/bash # Check which methods use aeronClient without mutex protection rg -n "aeronClient\.callFunc" source/bringauto/modules/ModuleManagerLibraryHandlerAsync.cpp -B5
🧹 Nitpick comments (5)
source/bringauto/modules/memory_management/source/memory_management.cpp (1)
17-21: Add null pointer validation for safety.While
delete[]on a nullptr is safe, the function should validatebuffer_pointeritself is non-null before dereferencing to prevent undefined behavior.Apply this diff:
void deallocate(struct buffer *buffer_pointer){ + if(buffer_pointer == nullptr){ + return; + } delete[] static_cast<char *>(buffer_pointer->data); buffer_pointer->data = nullptr; buffer_pointer->size_in_bytes = 0; }source/bringauto/modules/memory_management/CMakeLists.txt (1)
1-10: Consider using a unique project name.The CMakeLists.txt uses
PROJECT(ModuleGateway)which is the same name as the root project. While this may work, it's generally better practice to use a unique project name for submodules to avoid potential conflicts and improve clarity.Consider changing line 2 to:
-PROJECT(ModuleGateway) +PROJECT(ModuleGatewayMemoryManagement)Otherwise, the C++23 standard and dependency configuration look correct and consistent with the rest of the project.
source/bringauto/structures/ModuleLibrary.cpp (1)
16-31: Consider optimizing handler instantiation logic.The handler is created inside the loop for each module, which means the
moduleBinaryPath.empty()check is evaluated on every iteration even though the value doesn't change. Additionally, if using the Async handler, multiple instances are created with the samemoduleBinaryPath.While each module requires its own handler instance (which is correct), the handler type determination can be optimized.
Consider this optimization:
void ModuleLibrary::loadLibraries(const std::unordered_map<int, std::string> &libPaths, const std::string &moduleBinaryPath) { - std::shared_ptr<modules::IModuleManagerLibraryHandler> handler; + const bool useAsync = !moduleBinaryPath.empty(); for(auto const &[key, path]: libPaths) { - if (moduleBinaryPath.empty()) { - handler = std::make_shared<modules::ModuleManagerLibraryHandlerLocal>(); - } else { - handler = std::make_shared<modules::ModuleManagerLibraryHandlerAsync>(moduleBinaryPath); - } + auto handler = useAsync + ? std::static_pointer_cast<modules::IModuleManagerLibraryHandler>( + std::make_shared<modules::ModuleManagerLibraryHandlerAsync>(moduleBinaryPath)) + : std::static_pointer_cast<modules::IModuleManagerLibraryHandler>( + std::make_shared<modules::ModuleManagerLibraryHandlerLocal>()); handler->loadLibrary(path);Or more cleanly with a factory function approach to eliminate the repeated conditional.
test/source/ErrorAggregatorTests.cpp (1)
20-20: Test updates look correct, but consider adding tests for the Async handler.The existing tests have been properly updated to use
ModuleManagerLibraryHandlerLocal. However, based on the past review comment requesting unit tests, consider adding separate test cases forModuleManagerLibraryHandlerAsyncto ensure the async functionality works correctly and doesn't break existing behavior.Would you like me to generate test cases for the Async handler variant?
Also applies to: 31-31, 38-38
include/bringauto/settings/Constants.hpp (1)
95-103: Consider grouping Aeron-related constants into a dedicated structure.As noted in a previous review comment, creating an
AeronConstantsstruct (similar toMqttConstants) would make it clearer that these constants belong to Aeron and improve code organization.Apply this diff to create an organized structure:
+/** + * @brief Constants for Aeron communication +*/ +struct AeronConstants { + /** + * @brief base stream id for Aeron communication from Module Gateway to module binary + */ + static constexpr unsigned int to_module_stream_id_base { 10000 }; + + /** + * @brief base stream id for Aeron communication from module binary to Module Gateway + */ + static constexpr unsigned int to_gateway_stream_id_base { 20000 }; + + /** + * @brief Aeron IPC connection string + */ + static constexpr std::string_view connection { "aeron:ipc" }; +}; + -/** - * @brief base stream id for Aeron communication from Module Gateway to module binary - */ -constexpr unsigned int aeron_to_module_stream_id_base { 10000 }; - -/** - * @brief base stream id for Aeron communication from module binary to Module Gateway - */ -constexpr unsigned int aeron_to_gateway_stream_id_base { 20000 };Then move the
AERON_CONNECTIONconstant from theConstantsclass to this struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
CMLibStorage.cmake(1 hunks)CMakeLists.txt(4 hunks)README.md(2 hunks)cmake/Dependencies.cmake(1 hunks)include/bringauto/external_client/ErrorAggregator.hpp(3 hunks)include/bringauto/modules/Buffer.hpp(1 hunks)include/bringauto/modules/IModuleManagerLibraryHandler.hpp(1 hunks)include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp(1 hunks)include/bringauto/modules/ModuleManagerLibraryHandlerLocal.hpp(4 hunks)include/bringauto/modules/StatusAggregator.hpp(3 hunks)include/bringauto/settings/Constants.hpp(4 hunks)include/bringauto/settings/Settings.hpp(1 hunks)include/bringauto/structures/ModuleLibrary.hpp(3 hunks)main.cpp(1 hunks)resources/config/README.md(1 hunks)resources/config/default.json(1 hunks)resources/config/example.json(1 hunks)resources/config/for_docker.json(1 hunks)source/bringauto/external_client/ErrorAggregator.cpp(1 hunks)source/bringauto/modules/ModuleHandler.cpp(1 hunks)source/bringauto/modules/ModuleManagerLibraryHandlerAsync.cpp(1 hunks)source/bringauto/modules/ModuleManagerLibraryHandlerLocal.cpp(9 hunks)source/bringauto/modules/memory_management/CMakeLists.txt(1 hunks)source/bringauto/modules/memory_management/source/memory_management.cpp(1 hunks)source/bringauto/settings/SettingsParser.cpp(4 hunks)source/bringauto/structures/ModuleLibrary.cpp(2 hunks)test/CMakeLists.txt(1 hunks)test/include/ErrorAggregatorTests.hpp(2 hunks)test/include/ExternalConnectionTests.hpp(0 hunks)test/include/StatusAggregatorTests.hpp(2 hunks)test/include/testing_utils/ConfigMock.hpp(1 hunks)test/source/ErrorAggregatorTests.cpp(3 hunks)test/source/StatusAggregatorTests.cpp(4 hunks)
💤 Files with no reviewable changes (1)
- test/include/ExternalConnectionTests.hpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-03T11:53:12.434Z
Learnt from: mikusaq
PR: bringauto/mission-module#23
File: CMLibStorage.cmake:1-3
Timestamp: 2025-04-03T11:53:12.434Z
Learning: In CMLibStorage.cmake, the variable declaration `SET(STORAGE_LIST DEP)` is intentional and should not be changed to `SET(STORAGE_LIST_DEP)` despite appearing to be inconsistent with the later usage.
Applied to files:
CMLibStorage.cmake
📚 Learning: 2025-10-02T13:53:28.773Z
Learnt from: MarioIvancik
PR: bringauto/async-function-execution#3
File: examples/main_consumer.cpp:1-6
Timestamp: 2025-10-02T13:53:28.773Z
Learning: In the bringauto/async-function-execution repository, the header file `include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp` includes both `<iostream>` and `<stdexcept>`, so files that include `AsyncFunctionExecutor.hpp` already have access to these standard library headers transitively.
Applied to files:
include/bringauto/settings/Constants.hpp
📚 Learning: 2025-06-29T18:12:34.666Z
Learnt from: koudis
PR: bringauto/module-gateway#43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like `-DMODULE_GATEWAY_VERSION="1.3.3"`, ensuring the macro expands to a proper string literal in C++ code.
Applied to files:
CMakeLists.txttest/CMakeLists.txt
📚 Learning: 2024-10-03T11:24:43.894Z
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: source/bringauto/modules/devices/testing_device/testing_module_manager.cpp:25-34
Timestamp: 2024-10-03T11:24:43.894Z
Learning: In the `testing_device_aggregate_status` function for the testing device, status aggregation of current and new status is not necessary due to the module's simple logic, similar to the io-module which works without aggregation.
Applied to files:
source/bringauto/modules/ModuleManagerLibraryHandlerLocal.cpp
📚 Learning: 2024-10-31T10:45:48.494Z
Learnt from: MarioIvancik
PR: bringauto/module-gateway#38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in `include/bringauto/structures/LoggingSettings.hpp` are validated in `SettingsParser.cpp`, so additional validation within the structures is unnecessary.
Applied to files:
source/bringauto/settings/SettingsParser.cpp
🪛 Clang (14.0.6)
include/bringauto/structures/ModuleLibrary.hpp
[error] 3-3: 'bringauto/modules/IModuleManagerLibraryHandler.hpp' file not found
(clang-diagnostic-error)
include/bringauto/external_client/ErrorAggregator.hpp
[error] 3-3: 'bringauto/modules/IModuleManagerLibraryHandler.hpp' file not found
(clang-diagnostic-error)
include/bringauto/settings/Constants.hpp
[error] 3-3: 'bringauto/modules/Buffer.hpp' file not found
(clang-diagnostic-error)
source/bringauto/modules/memory_management/source/memory_management.cpp
[error] 1-1: 'new' file not found
(clang-diagnostic-error)
source/bringauto/modules/ModuleManagerLibraryHandlerLocal.cpp
[error] 1-1: 'bringauto/modules/ModuleManagerLibraryHandlerLocal.hpp' file not found
(clang-diagnostic-error)
source/bringauto/modules/ModuleManagerLibraryHandlerAsync.cpp
[error] 1-1: 'bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp' file not found
(clang-diagnostic-error)
include/bringauto/modules/IModuleManagerLibraryHandler.hpp
[error] 3-3: 'bringauto/modules/Buffer.hpp' file not found
(clang-diagnostic-error)
include/bringauto/modules/StatusAggregator.hpp
[error] 3-3: 'bringauto/modules/IModuleManagerLibraryHandler.hpp' file not found
(clang-diagnostic-error)
include/bringauto/modules/ModuleManagerLibraryHandlerLocal.hpp
[error] 3-3: 'bringauto/modules/IModuleManagerLibraryHandler.hpp' file not found
(clang-diagnostic-error)
include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp
[error] 3-3: 'bringauto/modules/IModuleManagerLibraryHandler.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (35)
test/CMakeLists.txt (1)
4-4: LGTM! C++ standard upgrade is consistent.The upgrade to C++23 aligns with the root CMakeLists.txt and the broader project-wide standard update.
CMLibStorage.cmake (1)
1-4: LGTM! CMCONF integration for Fleet Protocol.The CMCONF initialization for FLEET_PROTOCOL aligns with the new fleet-protocol dependencies and the async client architecture introduced in this PR.
source/bringauto/modules/ModuleHandler.cpp (1)
197-197: Minor readability improvement.The added empty line improves visual separation between the error handling block and the command retrieval logic.
CMakeLists.txt (6)
9-9: Version bump to 1.4.0 is appropriate.The version increment from 1.3.5 to 1.4.0 is suitable for the addition of async client functionality and new module management architecture.
19-19: LGTM! C++23 standard upgrade is consistent.The upgrade to C++23 is consistent with the test CMakeLists.txt and memory_management module.
57-59: New dependencies align with async client feature.The three new dependencies support the async client architecture:
- fleet-protocol-cxx-helpers-static 1.1.1 for C++ Fleet Protocol helpers
- aeron 1.48.6 for high-performance messaging
- async-function-execution-shared 1.0.0 for async execution patterns
These align with the PR objectives. However, verify the package naming consistency with Dependencies.cmake (see earlier comment on Dependencies.cmake lines 12-14).
64-64: Memory management subdirectory added.The new memory_management module is now part of the build. Ensure this module is necessary within Module Gateway rather than being provided by external dependencies. See the architectural concern raised in the memory_management.cpp review.
78-80: Link dependencies updated for async client support.The new link libraries complete the integration of async client functionality:
- fleet-protocol-cxx-helpers-static for protocol helpers
- async-function-execution-shared for async patterns
- memory_management for the local memory management implementation
Verify that the dependency resolution order is correct and there are no circular dependencies.
50-51: I'll verify the cxxopts update as well to complete the dependency compatibility check.No compatibility issues with updated dependencies; original review comment can be resolved.
Based on verification: The cxxopts 3.0.0 → 3.1.1 update includes only bug fixes and new features, and nlohmann_json 3.x series maintains backward compatibility. Both updates are safe and require no code changes.
resources/config/for_docker.json (1)
21-21: Verify consistency across all configuration files.The new "module-binary-path" field aligns with the async client feature. Please ensure that all configuration files (default.json, example.json) also include this field with appropriate defaults.
Run the following script to verify the field exists in all config files:
cmake/Dependencies.cmake (2)
12-14: Verify package naming consistency with CMakeLists.txt.There appears to be a naming discrepancy between Dependencies.cmake and CMakeLists.txt:
- Dependencies.cmake line 12:
fleet-protocol-cpp v1.1.1- CMakeLists.txt line 57:
fleet-protocol-cxx-helpers-static 1.1.1And similarly:
- Dependencies.cmake line 14:
async-function-execution v1.0.0- CMakeLists.txt line 59:
async-function-execution-shared 1.0.0Please verify these are the correct package names or if there's an inconsistency that needs resolution.
6-6: Consider upgrading cxxopts to latest stable version v3.3.1.The latest stable release of cxxopts is v3.3.1 (published May–Jun 2025), while this PR uses v3.1.1. No widely-reported critical bugs are specific to v3.1.1, but the version is notably behind current releases. Additionally, v3 introduced breaking changes: the parser no longer modifies argc/argv and ParseResult no longer depends on the parser—verify the codebase is compatible with these changes.
source/bringauto/modules/memory_management/source/memory_management.cpp (1)
1-21: Consider the architecture: why is memory_management in Module Gateway?A previous reviewer (koudis) questioned why memory_management is defined within Module Gateway. This is a valid architectural concern. Consider whether:
- This should be part of the fleet-protocol library instead
- It duplicates functionality that might already exist in fleet-protocol-cpp
- The Module Gateway should depend on external memory management rather than implementing its own
include/bringauto/modules/ModuleManagerLibraryHandlerLocal.hpp (1)
12-73: LGTM! Clean refactoring to interface-based design.The migration from a standalone class to inheriting from
IModuleManagerLibraryHandleris well-executed. All public methods are correctly marked withoverride, and the class structure remains intact.source/bringauto/modules/ModuleManagerLibraryHandlerLocal.cpp (1)
1-222: LGTM! Consistent class renaming throughout the implementation.All method implementations have been correctly updated to use the new
ModuleManagerLibraryHandlerLocalclass name. The logic remains unchanged, making this a safe refactoring.resources/config/default.json (1)
17-17: LGTM! Configuration schema extended appropriately.The new
module-binary-pathfield is correctly added with an empty default value, maintaining consistency with the configuration structure.resources/config/example.json (1)
23-23: LGTM! Example configuration updated consistently.The
module-binary-pathfield is added in the same location as indefault.json, maintaining consistency across configuration files.test/include/testing_utils/ConfigMock.hpp (1)
108-108: LGTM! Config mock updated to include new field.The generated configuration string now includes
module-binary-pathwith an empty value, consistent with the example configurations. If future tests need to exercise scenarios with a non-emptymodule-binary-path, consider adding it as a configurable member of theConfigstruct.main.cpp (2)
70-71: LGTM!The early instantiation of
moduleLibraryis appropriate for the subsequent library loading operations.
74-74: Verify thatmoduleBinaryPathis properly configured in Settings.The new parameter enables async client support. Ensure that
context->settings->moduleBinaryPathis properly initialized and validated during settings parsing.source/bringauto/structures/ModuleLibrary.cpp (1)
2-3: LGTM!The new includes are necessary for the Local and Async handler implementations.
test/source/ErrorAggregatorTests.cpp (1)
3-3: LGTM!The include is correctly updated to use the Local handler variant for testing.
test/include/ErrorAggregatorTests.hpp (1)
6-6: LGTM!The test fixture correctly depends on the
IModuleManagerLibraryHandlerinterface, allowing flexibility in the concrete implementation used. This follows the dependency inversion principle.Also applies to: 34-34
source/bringauto/external_client/ErrorAggregator.cpp (1)
11-11: LGTM!The parameter type correctly updated to accept the
IModuleManagerLibraryHandlerinterface, enabling support for both Local and Async handler implementations.include/bringauto/modules/Buffer.hpp (1)
21-22: LGTM!The friend declarations are correctly updated to grant both
ModuleManagerLibraryHandlerLocalandModuleManagerLibraryHandlerAsyncaccess to the private Buffer constructor, as friendship is not inherited through interfaces.test/include/StatusAggregatorTests.hpp (1)
7-7: LGTM!The test fixture correctly depends on the
IModuleManagerLibraryHandlerinterface, consistent with the updated architecture.Also applies to: 46-46
README.md (2)
60-60: LGTM!Capitalization improvement for consistency with other headings.
32-42: Verify that new dependencies are required and properly integrated.The documentation adds several new dependencies (pahomqtt, pahomqttcpp, zlib, async-function-execution) and bumps versions for existing ones. Ensure that:
- New dependencies are actually used in the codebase
- Version requirements are accurate
- CMakeLists.txt is updated to include these dependencies
include/bringauto/external_client/ErrorAggregator.hpp (1)
3-3: LGTM! Interface migration is correct.The migration from concrete
ModuleManagerLibraryHandlerto theIModuleManagerLibraryHandlerinterface is consistent and allows for both Local and Async handler implementations.Also applies to: 28-28, 114-114
include/bringauto/structures/ModuleLibrary.hpp (2)
16-16: LGTM! Default parameter addresses previous concern.The addition of a default empty string for
moduleBinaryPathparameter properly addresses the previous review comment about not forcing the path when using standard (non-async) loading. This allows flexibility for both loading modes.Also applies to: 24-26
35-35: LGTM! Interface migration is correct.The change to
IModuleManagerLibraryHandlerinterface type is consistent with the architectural refactor and enables polymorphic handler selection.test/source/StatusAggregatorTests.cpp (1)
3-3: LGTM! Test updates are consistent.The migration to
ModuleManagerLibraryHandlerLocalin tests is appropriate, as tests should use concrete implementations. All usages are consistently updated.Also applies to: 46-46, 64-64, 99-99
include/bringauto/modules/StatusAggregator.hpp (1)
3-3: LGTM! Interface migration is correct.The migration from concrete
ModuleManagerLibraryHandlerto theIModuleManagerLibraryHandlerinterface is consistent throughout the class and enables polymorphic handler usage.Also applies to: 22-25, 189-189
source/bringauto/settings/SettingsParser.cpp (2)
98-101: LGTM! Proper validation for optional moduleBinaryPath.The validation correctly checks for the existence of
moduleBinaryPathonly when it's non-empty, which aligns with the optional nature of the async module binary feature.
155-157: LGTM! Safer JSON parsing methods.The use of
at()andget_to()provides better error handling by throwing exceptions for missing keys, which is more robust than the bracket operator that would insert default values.Also applies to: 161-164, 184-184
| std::span<const uint8_t> serialize() const { | ||
| size_t total_size = sizeof(int) + buffer.size_in_bytes; | ||
| uint8_t* data = new uint8_t[total_size]; | ||
| std::memcpy(data, &returnCode, sizeof(int)); | ||
| std::memcpy(data + sizeof(int), buffer.data, buffer.size_in_bytes); | ||
| return {data, total_size}; | ||
| } |
There was a problem hiding this comment.
Critical memory leak in serialize().
The serialize() method allocates memory with new uint8_t[] but returns a std::span that doesn't convey ownership. The caller has no way to delete this memory, resulting in a leak on every call.
Consider using std::vector<uint8_t> for automatic memory management:
- std::span<const uint8_t> serialize() const {
+ std::vector<uint8_t> serialize() const {
size_t total_size = sizeof(int) + buffer.size_in_bytes;
- uint8_t* data = new uint8_t[total_size];
+ std::vector<uint8_t> data(total_size);
- std::memcpy(data, &returnCode, sizeof(int));
- std::memcpy(data + sizeof(int), buffer.data, buffer.size_in_bytes);
- return {data, total_size};
+ std::memcpy(data.data(), &returnCode, sizeof(int));
+ std::memcpy(data.data() + sizeof(int), buffer.data, buffer.size_in_bytes);
+ return data;
}Similarly for ConvertibleBuffer::serialize() if it allocates dynamically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::span<const uint8_t> serialize() const { | |
| size_t total_size = sizeof(int) + buffer.size_in_bytes; | |
| uint8_t* data = new uint8_t[total_size]; | |
| std::memcpy(data, &returnCode, sizeof(int)); | |
| std::memcpy(data + sizeof(int), buffer.data, buffer.size_in_bytes); | |
| return {data, total_size}; | |
| } | |
| std::vector<uint8_t> serialize() const { | |
| size_t total_size = sizeof(int) + buffer.size_in_bytes; | |
| std::vector<uint8_t> data(total_size); | |
| std::memcpy(data.data(), &returnCode, sizeof(int)); | |
| std::memcpy(data.data() + sizeof(int), buffer.data, buffer.size_in_bytes); | |
| return data; | |
| } |
🤖 Prompt for AI Agents
In include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp around lines
21-27, serialize() currently allocates a raw uint8_t[] and returns a std::span,
leaking memory because ownership is not transferred; replace the raw allocation
with an owned container (prefer std::vector<uint8_t>) and change the method to
return std::vector<uint8_t> (or another owning type), copy the header and buffer
into that vector, and return it; apply the same fix to any
ConvertibleBuffer::serialize() that allocates dynamically so ownership is clear
and memory is automatically freed.
source/bringauto/modules/memory_management/source/memory_management.cpp
Outdated
Show resolved
Hide resolved
| if (!moduleBinaryProcess_.valid()) { | ||
| throw std::runtime_error { "Failed to start module binary " + moduleBinaryPath_ }; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::seconds(1)); // TODO Not sure how much time is needed. |
There was a problem hiding this comment.
Replace arbitrary sleep with proper startup verification.
The TODO comment acknowledges that the sleep duration is uncertain. Consider implementing a proper health check or ready signal from the module binary instead of an arbitrary sleep.
Possible approaches:
- Poll the Aeron client for connectivity with a timeout
- Have the module binary send a ready signal
- Use a configurable startup timeout with retry logic
|
Summary by CodeRabbit
New Features
Improvements