Add module function definitions #8
Conversation
WalkthroughAdds async module function definitions and executor, introduces convertible buffer serialization types, updates CMake (C++ standard → 23, new package finds, CMCONF init, added dependencies), and adds a unit test validating async function IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ModuleFuncExec as ModuleFunctionExecutor
participant AsyncLib as async_function_execution
participant ModuleImpl as ModuleImplementation
Note right of ModuleFuncExec `#ddeeff`: New ModuleFunctionExecutor + FunctionDefinitions
Caller->>ModuleFuncExec: invoke(functionId, args)
ModuleFuncExec->>AsyncLib: schedule(FunctionDefinition, args)
AsyncLib->>ModuleImpl: execute implementation
ModuleImpl-->>AsyncLib: result
AsyncLib-->>ModuleFuncExec: deliver result
ModuleFuncExec-->>Caller: return future/result
sequenceDiagram
participant User
participant Convertible as ConvertibleBuffer/ConvertibleBufferReturn
participant Bytes as ByteSpan
User->>Convertible: construct/populate
Convertible->>Bytes: serialize() -> span over allocated bytes
User->>Convertible: deserialize(span) -> reconstruct internal ::buffer (allocates/copies)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-03T08:51:49.215ZApplied to files:
📚 Learning: 2025-07-18T14:05:04.696ZApplied to files:
🧬 Code graph analysis (2)include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp (3)
include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp (2)
🪛 Clang (14.0.6)include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found (clang-diagnostic-error) include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found (clang-diagnostic-error) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/CMakeLists.txt (1)
4-4:CMAKE_CXX_STANDARD 23in test/CMakeLists.txt is redundantVerification confirms that
CMAKE_CXX_STANDARD 23is already set globally in the root CMakeLists.txt (line 5), so the repeated setting in test/CMakeLists.txt (line 4) is unnecessary. In CMake, the cache variable propagates to all subdirectories. Consider removing it from test/CMakeLists.txt and the related redundant setting in test/lib/memory_management/CMakeLists.txt (line 4).include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp (1)
19-20: Prefer fixed-width integer for serialized return codeSince the serialized format treats the return code as a fixed 4‑byte field, consider using
std::int32_t(from<cstdint>) forreturnCodeandsizeof(returnCode)instead of hardcodingint/sizeof(int). This makes the on-wire representation more explicit and robust across platforms.Also applies to: 24-28
include/bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp (1)
3-7: Optional: make the header self-contained wrtuint32_tThis header uses
uint32_tin the argument lists but relies on transitively included headers to pull in<cstdint>. Adding an explicit#include <cstdint>here would make the header self-contained and less fragile to future include changes.Also applies to: 17-63
test/source/AsyncModuleFunctionDefinitionsTest.cpp (1)
1-18: ID verification test looks good; consider also checkingmoduleFunctionListorderingThe test correctly asserts that each async module function definition has the expected ID 0–8, which will catch accidental renumbering. As an optional enhancement, you could also assert that
moduleFunctionListexposes the definitions in the same order (e.g., by iterating over it and checkingid.value), so changes in list construction can’t silently diverge from the individual definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CMLibStorage.cmake(1 hunks)CMakeLists.txt(3 hunks)cmake/Dependencies.cmake(1 hunks)include/bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp(1 hunks)include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp(1 hunks)include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp(1 hunks)test/CMakeLists.txt(1 hunks)test/lib/memory_management/CMakeLists.txt(1 hunks)test/source/AsyncModuleFunctionDefinitionsTest.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: mikusaq
Repo: bringauto/packager PR: 24
File: example/package/fleet-protocol-internal-client/internal_client_debug.json:23-23
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The repository URL for 'fleet-protocol-internal-client' is 'https://github.com/bringauto/internal-client-cpp.git', not 'https://github.com/bringauto/fleet-protocol-internal-client.git'.
📚 Learning: 2024-10-09T05:03:50.249Z
Learnt from: mikusaq
Repo: bringauto/packager PR: 24
File: example/package/fleet-protocol-internal-client/internal_client_debug.json:23-23
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The repository URL for 'fleet-protocol-internal-client' is 'https://github.com/bringauto/internal-client-cpp.git', not 'https://github.com/bringauto/fleet-protocol-internal-client.git'.
Applied to files:
cmake/Dependencies.cmaketest/lib/memory_management/CMakeLists.txtCMakeLists.txttest/CMakeLists.txt
📚 Learning: 2024-10-03T17:41:56.410Z
Learnt from: Mayt34
Repo: bringauto/transparent-module PR: 1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-03T17:41:56.410Z
Learning: In the `transparent_module` project, all modules should be built by default; `FLEET_PROTOCOL_BUILD_MODULE_GATEWAY` and `FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER` should be set to ON by default.
Applied to files:
cmake/Dependencies.cmakeCMLibStorage.cmaketest/lib/memory_management/CMakeLists.txtCMakeLists.txttest/CMakeLists.txt
📚 Learning: 2025-04-03T11:53:12.434Z
Learnt from: mikusaq
Repo: bringauto/mission-module PR: 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-11-03T08:51:49.215Z
Learnt from: MarioIvancik
Repo: bringauto/module-gateway PR: 47
File: include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp:47-50
Timestamp: 2025-11-03T08:51:49.215Z
Learning: In the bringauto/module-gateway repository, the `ConvertibleBuffer::deserialize()` method in `include/bringauto/modules/ModuleManagerLibraryHandlerAsync.hpp` intentionally uses shallow copying (via const_cast) instead of deep copying for performance reasons. This avoids needless data copying, and the data validity period is documented to ensure proper lifetime management.
Applied to files:
include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hppinclude/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp
📚 Learning: 2025-07-18T14:05:04.696Z
Learnt from: MarioIvancik
Repo: bringauto/module-gateway PR: 45
File: include/bringauto/common_utils/ProtobufUtils.hpp:115-115
Timestamp: 2025-07-18T14:05:04.696Z
Learning: In the Module Gateway codebase, the `modules::Buffer` class has a `getStructBuffer()` method that returns a `struct ::buffer` by value (not by reference). This struct contains pointers to the actual buffer data, so even when the Buffer object is passed as `const modules::Buffer&`, the returned struct can still be used to modify the buffer contents via memcpy operations. This design pattern allows const-correctness at the C++ wrapper level while maintaining compatibility with C library interfaces that need mutable buffer access.
Applied to files:
include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hppinclude/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp
📚 Learning: 2024-10-09T05:03:50.249Z
Learnt from: Mayt34
Repo: bringauto/transparent-module PR: 1
File: CMakeLists.txt:46-66
Timestamp: 2024-10-09T05:03:50.249Z
Learning: In this project, using `GLOB_RECURSE` in `CMakeLists.txt` is preferred for source file listing to allow automatic inclusion of new source files without modifying the file.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-10-02T13:53:35.438Z
Learnt from: MarioIvancik
Repo: bringauto/async-function-execution PR: 3
File: include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp:172-185
Timestamp: 2025-10-02T13:53:35.438Z
Learning: In the async-function-execution project tests (test/include/AsyncFunctionExecutorTests.hpp), the MockClient's connect() method is a no-op that always returns 0, so checking return values in test setup is not required.
Applied to files:
test/source/AsyncModuleFunctionDefinitionsTest.cpp
📚 Learning: 2025-10-02T13:53:28.773Z
Learnt from: MarioIvancik
Repo: bringauto/async-function-execution PR: 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/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp
🧬 Code graph analysis (2)
include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp (2)
include/bringauto/fleet_protocol/cxx/StringAsBuffer.hpp (1)
buff(20-20)include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp (2)
bytes(31-40)bytes(31-31)
include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp (2)
include/bringauto/fleet_protocol/cxx/StringAsBuffer.hpp (1)
buff(20-20)include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp (2)
bytes(24-27)bytes(24-24)
🪛 Clang (14.0.6)
include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp
[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found
(clang-diagnostic-error)
test/source/AsyncModuleFunctionDefinitionsTest.cpp
[error] 1-1: 'bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp' file not found
(clang-diagnostic-error)
include/bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp
[error] 3-3: 'bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp' file not found
(clang-diagnostic-error)
include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp
[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (4)
test/lib/memory_management/CMakeLists.txt (1)
4-4: C++23 standard for memory_management tests aligns with project-wide upgradeVerified that
CMAKE_CXX_STANDARD 23in test/lib/memory_management/CMakeLists.txt (line 4) is consistent with the project-wide C++23 adoption (also set in ./CMakeLists.txt:5 and ./test/CMakeLists.txt:4). No issues found.CMLibStorage.cmake (1)
1-3: Approved—CMCONF initialization verified as safeVerification confirms
CMCONF_INIT_SYSTEM(FLEET_PROTOCOL)is called exactly once in the codebase (line 3 of CMLibStorage.cmake). No duplicate initializations detected. The code structure is sound and the pattern is consistent with standard CMake practice.CMakeLists.txt (1)
5-7: Build config changes (C++23 + new deps) look consistentThe move to C++23, the helpers version bump, the new async-function-execution-shared link, and switching tests to
INCLUDE(test/CMakeLists.txt)all look coherent with the new async module API; nothing blocking from this CMake snippet. Please just ensure your CI/toolchains have C++23 enabled and thataeron 1.48.6andasync-function-execution-shared 1.0.0are available in all target environments.Also applies to: 43-44, 52-53, 57-60, 81-84
include/bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp (1)
11-63: Async function definitions and executor wiring look correct; verify against module contractThe nine
FunctionDefinitioninstances, theModuleFunctionExecutoralias, andmoduleFunctionListare wired consistently: IDs are sequential 0–8, return/argument types match the described module functions, and the order of types inModuleFunctionExecutorandmoduleFunctionListis aligned with the IDs and with the unit test.Please double-check that:
- The IDs 0–8 and function ordering exactly match what the module gateway and module binary expect.
- The chosen C++ types (
int,uint32_t,ConvertibleBuffer,ConvertibleBufferReturn) are compatible with the underlying C/module APIs used by async-function-execution.Also applies to: 68-78, 83-103
| BA_PACKAGE_LIBRARY(aeron v1.48.6) | ||
| BA_PACKAGE_LIBRARY(async-function-execution v1.0.0) |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure BA package names match the CMake find_package usage
Adding aeron v1.48.6 and async-function-execution v1.0.0 here looks consistent with the PR description, but please confirm that:
- The BA package names (
aeron,async-function-execution) correspond to the actual package IDs in your package tracker. - The name used here for async-function-execution matches the one used in
find_package(e.g.,async-function-execution-sharedin CMakeLists, if that’s what you use), so CMLIB resolves the same artifact.
You can grep for how these libraries are consumed:
🏁 Script executed:
#!/bin/bash
rg -n "async-function-execution" .
rg -n "aeron" .Length of output: 392
Fix package name mismatch for async-function-execution-shared
The verification reveals a critical inconsistency: cmake/Dependencies.cmake:4 defines BA_PACKAGE_LIBRARY(async-function-execution v1.0.0), but CMakeLists.txt:44 calls FIND_PACKAGE(async-function-execution-shared 1.0.0 REQUIRED).
The BA package name must match what find_package resolves. Change line 4 in cmake/Dependencies.cmake from async-function-execution to async-function-execution-shared. The aeron entry (line 3) is correctly named.
🤖 Prompt for AI Agents
In cmake/Dependencies.cmake around lines 3 to 4, the BA package name for the
async library is incorrect: line 4 currently registers
"async-function-execution" but CMakeLists.txt expects
"async-function-execution-shared"; update line 4 to use
BA_PACKAGE_LIBRARY(async-function-execution-shared v1.0.0) so the BA package
name matches the FIND_PACKAGE call, leaving the aeron entry unchanged.
| #include <fleet_protocol/common_headers/device_management.h> | ||
|
|
||
| #include <cstdint> | ||
| #include <span> | ||
|
|
||
|
|
||
|
|
||
| namespace bringauto::fleet_protocol::cxx { | ||
|
|
||
| /** | ||
| * @brief ConvertibleBuffer is a helper class to convert a fleet_protocol 'buffer' struct | ||
| * to/from a byte span for serialization/deserialization. | ||
| */ | ||
| struct ConvertibleBuffer final { | ||
| struct ::buffer buffer {}; | ||
| ConvertibleBuffer() = default; | ||
| ConvertibleBuffer(struct ::buffer buff) : buffer(buff) {} | ||
|
|
||
| std::span<const uint8_t> serialize() const { | ||
| return std::span {reinterpret_cast<const uint8_t *>(buffer.data), buffer.size_in_bytes}; | ||
| } | ||
| void deserialize(std::span<const uint8_t> bytes) { | ||
| buffer.data = const_cast<uint8_t *>(bytes.data()); | ||
| buffer.size_in_bytes = bytes.size(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify shallow, non-owning deserialize semantics and verify header include path
-
The
deserializemethod:void deserialize(std::span<const uint8_t> bytes) { buffer.data = const_cast<uint8_t *>(bytes.data()); buffer.size_in_bytes = bytes.size(); }
performs a shallow conversion:
bufferends up pointing directly into thebytesspan with no copy. This is consistent with the pattern used in related projects (const-correct wrapper over a C buffer plus const_cast for performance), but it means:ConvertibleBufferdoes not own the memory.buffer.databecomes invalid as soon as the source span’s underlying storage is destroyed or modified.
I’d strongly recommend making this explicit in the doc comment (e.g., that
deserializealiases the span and that the caller must ensure the lifetime of the underlying data) to avoid misuse and accidental dangling pointers.
Based on learnings -
Static analysis reports
'fleet_protocol/common_headers/device_management.h' file not foundat Line 3. That may just be a tooling include-path issue, but please double-check that:- The header really lives under
fleet_protocol/common_headers/device_management.hin your install layout, and - Your CMake includes the appropriate include directories when building this target.
If the installed path differs (e.g., an extra
bringauto/prefix), updating the include or include dirs here will prevent build failures. - The header really lives under
To sanity-check usage and ensure expectations around ownership are clear, you can locate all call sites:
🏁 Script executed:
#!/bin/bash
rg -n "ConvertibleBuffer" include testLength of output: 2829
I need to gather more information. Let me search for actual deserialize calls and verify the include path.
Now I'll search the repository directly for the necessary information:
Based on my analysis of the code and the learnings from the module-gateway project, I can now generate the rewritten review comment:
Add documentation for shallow, non-owning deserialize semantics and verify include path
The deserialize method creates a non-owning alias to the input span via const_cast. While this pattern is intentional for performance, the doc comment should explicitly state:
ConvertibleBufferdoes not own the aliased memory- The caller must ensure the underlying span's data remains valid for the
ConvertibleBuffer's lifetime
For the include path at line 3 (<fleet_protocol/common_headers/device_management.h>): verify your CMake configuration includes the correct directories when building this target. If the header is installed at a different path, update either the include or the CMake include_directories accordingly to prevent build failures.
🏁 Script executed:
#!/bin/bash
# Search for deserialize method calls
rg -n "\.deserialize\(" --type cpp include test src 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find device_management.h file
fd -t f "device_management\.h" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check for CMakeLists or include_directories configuration
fd -t f "CMakeLists\.txt" | head -5Length of output: 76
🏁 Script executed:
#!/bin/bash
# Look for the buffer struct definition
rg -n "struct.*buffer" --type c --type h -A 5 include | head -50Length of output: 1709
🏁 Script executed:
#!/bin/bash
# Check if there's a fleet_protocol subdirectory
find . -type d -name "fleet_protocol" 2>/dev/null | head -10Length of output: 68
I need to check the CMakeLists configuration to understand the include path setup and where device_management comes from:
🏁 Script executed:
#!/bin/bash
# Check main CMakeLists.txt for include configuration
cat CMakeLists.txt | head -80Length of output: 2559
🏁 Script executed:
#!/bin/bash
# Search for device_management references in build files
rg -i "device_management" --type cmake --type txt 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for external dependencies or include_directories declarations
rg -n "include_directories|find_package|add_subdirectory" CMakeLists.txtLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if fleet_protocol is an external dependency or package
rg -n "fleet_protocol\|fleet-protocol" CMakeLists.txtLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for common_headers directory anywhere in the repo
find . -type d -name "common_headers" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for buffer struct definition in any header file
rg -n "^\s*struct.*buffer.*{" --type h includeLength of output: 105
🏁 Script executed:
#!/bin/bash
# Check for pkg-config or cmake finds related to fleet-protocol
rg -n "pkg_check|find_package|PkgConfig" CMakeLists.txtLength of output: 0
🏁 Script executed:
#!/bin/bash
# Examine project structure for dependencies or installed files
ls -la | grep -E "\.cmake|\.pc|conanfile|requirements"Length of output: 67
🏁 Script executed:
#!/bin/bash
# Check if the include path exists but just with different structure
find . -path "*fleet_protocol*" -name "*.h" 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search all headers in include directory to see what's included
fd -e h . include/bringauto/fleet_protocol/cxx | xargs rg -l "device_management"Length of output: 173
🏁 Script executed:
#!/bin/bash
# Check what headers are in the common_headers or device_management areas if they exist
find . -path "*fleet_protocol*common*" -o -path "*device_management*" 2>/dev/nullLength of output: 0
Let me verify the external package structure one more time:
🏁 Script executed:
#!/bin/bash
# Check DeviceID.hpp to see how it includes device_management
cat include/bringauto/fleet_protocol/cxx/DeviceID.hpp | head -20Length of output: 483
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about the external dependency
rg -n "fleet-protocol-interface\|common-headers" --type cmakeLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a conan file or requirements
find . -name "conanfile*" -o -name "requirements.txt" -o -name ".conan*" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the Dependencies.cmake to understand external packages
cat cmake/Dependencies.cmake 2>/dev/null | head -50Length of output: 314
Add documentation for shallow, non-owning deserialize semantics
The deserialize method creates a non-owning alias to the input span via const_cast. While this pattern is intentional for performance, the doc comment should explicitly state:
ConvertibleBufferdoes not own the aliased memory- The caller must ensure the underlying span's data remains valid for the
ConvertibleBuffer's lifetime
The include path <fleet_protocol/common_headers/device_management.h> is correct—it's provided by the external fleet-protocol-interface dependency that the CMake configuration properly fetches and links.
🧰 Tools
🪛 Clang (14.0.6)
[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp around lines 3 to
27, the deserialize method currently creates a shallow, non-owning alias to the
input span via const_cast without any documentation; update the header comment
for deserialize (and/or class-level doc) to explicitly state that
ConvertibleBuffer does not take ownership of the aliased memory, that
deserialize performs a non-owning aliasing (uses const_cast) for performance,
and that the caller is responsible for ensuring the input span's backing storage
remains valid for the entire lifetime of the ConvertibleBuffer instance.
| struct ConvertibleBufferReturn final { | ||
| int returnCode {}; | ||
| struct ::buffer buffer {}; | ||
| ConvertibleBufferReturn() = default; | ||
| ConvertibleBufferReturn(int code, struct ::buffer buff) : returnCode(code), buffer(buff) {} | ||
|
|
||
| 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}; | ||
| } | ||
| void deserialize(std::span<const uint8_t> bytes) { | ||
| auto size = bytes.size(); | ||
| if (size < sizeof(int)) return; | ||
| std::memcpy(&returnCode, bytes.data(), sizeof(int)); | ||
| size -= sizeof(int); | ||
| buffer.data = new uint8_t[size]; | ||
| buffer.size_in_bytes = size; | ||
| std::memcpy(buffer.data, bytes.data() + sizeof(int), size); | ||
| buffer.size_in_bytes = size; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix ownership/lifetime in serialize/deserialize (current code leaks and risks double-ownership)
serialize() allocates with new[] and returns a std::span<const uint8_t> that has no way to free the memory, so every call leaks. deserialize() also allocates with new[] into buffer.data without any corresponding delete/free and will leak or overwrite pointers if called multiple times. This is also inconsistent with the existing shallow-aliasing pattern used by ConvertibleBuffer::deserialize.
A safer pattern is:
- Use an internal owning buffer for serialization, e.g. a
mutable std::vector<uint8_t>member, and return a span over that. - In
deserialize(), avoid allocating and instead shallow-copy intobuffer(asConvertibleBufferdoes), pointing into the suppliedbytesspan:
-struct ConvertibleBufferReturn final {
- int returnCode {};
- struct ::buffer buffer {};
+struct ConvertibleBufferReturn final {
+ int returnCode {};
+ struct ::buffer buffer {};
+ mutable std::vector<uint8_t> serializedData{};
@@
- 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};
- }
- void deserialize(std::span<const uint8_t> bytes) {
- auto size = bytes.size();
- if (size < sizeof(int)) return;
- std::memcpy(&returnCode, bytes.data(), sizeof(int));
- size -= sizeof(int);
- buffer.data = new uint8_t[size];
- buffer.size_in_bytes = size;
- std::memcpy(buffer.data, bytes.data() + sizeof(int), size);
- buffer.size_in_bytes = size;
- }
+ std::span<const uint8_t> serialize() const {
+ const auto headerSize = sizeof(returnCode);
+ const auto payloadSize = buffer.size_in_bytes;
+ serializedData.resize(headerSize + payloadSize);
+ std::memcpy(serializedData.data(), &returnCode, headerSize);
+ std::memcpy(serializedData.data() + headerSize, buffer.data, payloadSize);
+ return {serializedData.data(), serializedData.size()};
+ }
+
+ void deserialize(std::span<const uint8_t> bytes) {
+ const auto size = bytes.size();
+ const auto headerSize = sizeof(returnCode);
+ if (size < headerSize) {
+ return;
+ }
+
+ std::memcpy(&returnCode, bytes.data(), headerSize);
+ const auto payloadSize = size - headerSize;
+ buffer.data = const_cast<uint8_t *>(bytes.data() + headerSize);
+ buffer.size_in_bytes = payloadSize;
+ }This eliminates the leaks and matches the shallow-copy semantics already used for ConvertibleBuffer, assuming the caller guarantees the lifetime of the backing storage for the bytes span. Based on learnings.
I'll verify the review comment by examining the codebase structure and comparing the current implementation against the pattern used elsewhere.
🏁 Script executed:
#!/bin/bash
# 1. Locate the file and confirm it exists
fd -t f ConvertibleBufferReturn.hpp
# 2. Find ConvertibleBuffer to compare patterns
fd -t f ConvertibleBuffer.hpp
# 3. Search for ::buffer struct definition
rg -n "struct\s+::?buffer" --type=cpp --type=hpp -A 5Length of output: 151
🏁 Script executed:
#!/bin/bash
# 4. Check for destructors or cleanup in ConvertibleBufferReturn
rg -n "ConvertibleBufferReturn" --type=cpp --type=hpp -C 3
# 5. Look for usages of serialize() to understand lifetime
rg -n "\.serialize\(\)" --type=cpp --type=hpp -C 2Length of output: 55
🏁 Script executed:
#!/bin/bash
# 6. Examine ConvertibleBuffer::deserialize implementation to confirm shallow aliasing
ast-grep --pattern $'struct ConvertibleBuffer {
$$$
deserialize($_) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# View the two key files directly
echo "=== ConvertibleBufferReturn.hpp ==="
cat include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp
echo ""
echo "=== ConvertibleBuffer.hpp ==="
cat include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hppLength of output: 2142
🏁 Script executed:
#!/bin/bash
# Search for deserialize patterns without type restrictions
echo "=== Usages of deserialize ==="
rg -n "deserialize" -C 3
echo ""
echo "=== Struct ::buffer definition ==="
rg -n "struct.*buffer" -C 2Length of output: 5393
🏁 Script executed:
#!/bin/bash
# Check for usage of serialize and any cleanup/deleter patterns
echo "=== Serialize usages ==="
rg -n "serialize\(\)" -C 2
echo ""
echo "=== Check for delete or std::unique_ptr around buffer ==="
rg -n "delete.*buffer|std::unique_ptr.*buffer" -C 2Length of output: 1653
Fix memory leaks in serialize() and deserialize() — use internal buffer and shallow aliasing
The review comment correctly identifies critical memory leaks:
serialize()allocates withnew[]and returns astd::span<const uint8_t>with no deleter; every call leaks the heap-allocated memory.deserialize()allocates withnew[]intobuffer.dataand never deletes; repeated calls leak and risk double-free.
Verified file state matches the snippet exactly. The implementation also diverges from the existing ConvertibleBuffer pattern, which intentionally uses shallow aliasing (const_cast) in deserialize() and no allocation in serialize() — an approach already documented in the codebase as intentional for performance.
The proposed fix—internal owning mutable std::vector<uint8_t> for serialization and shallow aliasing in deserialize()—is sound and aligns with the established shallow-copy semantics used by ConvertibleBuffer. This ensures proper lifetime management without repeated allocations and avoids dangling pointers.
🤖 Prompt for AI Agents
In include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp around lines
18-40, the current serialize() and deserialize() leak heap memory and diverge
from the project's shallow-aliasing ConvertibleBuffer pattern; replace the raw
new[] allocations by adding a mutable internal owning std::vector<uint8_t>
member used for serialization (fill it with sizeof(int)+buffer.size_in_bytes,
copy returnCode then buffer bytes into it, and return a std::span pointing at
vector.data()), and change deserialize() to perform shallow aliasing (do not
allocate; set buffer.data to point into the incoming span + sizeof(int) via
const_cast<uint8_t*> and set buffer.size_in_bytes accordingly) so lifetimes
match the documented pattern and no raw new/delete is used.
| #include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> | ||
|
|
||
|
|
||
| namespace bringauto::fleet_protocol::cxx { |
There was a problem hiding this comment.
please move it to different namesapace. not cx.
Added module function definitions required for async function execution on module gateway and module binary
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.