First proposed release #3
Conversation
WalkthroughBumps C++ standard and project version; removes nlohmann_json; replaces JSON-string function configs with typed structures::FunctionConfigs; changes AsyncFunctionExecutor APIs (connect() → int, callFunc → std::expected, per-function timeouts and call-in-progress tracking); makes client wait APIs timeout-aware; refactors idle strategy; splits examples into producer/consumer/driver; updates CMake and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Exec as AsyncFunctionExecutor
participant Client as ClientInterface/AeronClient
rect rgb(250,250,255)
note right of App: Init & connect
App->>Exec: AsyncFunctionExecutor(config, FunctionList, client?)
App->>Exec: connect()
Exec->>Client: connect(...)
Client-->>Exec: int status
Exec-->>App: return status (0 == ok)
end
sequenceDiagram
autonumber
actor App
participant Exec as AsyncFunctionExecutor (Producer)
participant Client as ClientInterface
rect rgb(245,255,245)
note right of App: Producer call flow (per-function timeout)
App->>Exec: callFunc(FunctionDef, args...)
Exec->>Client: sendMessage(callChannel, encodedArgs)
Exec->>Client: waitForMessage(returnChannel, timeout = fn.timeout || default)
alt message received
Client-->>Exec: span(bytes)
Exec-->>App: std::expected with value
else timeout / empty
Client-->>Exec: empty span
Exec-->>App: std::expected with CallError (Timeout/…)
end
end
sequenceDiagram
autonumber
actor App
participant Exec as AsyncFunctionExecutor (Consumer)
participant Client as ClientInterface
participant Handler as UserCode
App->>Exec: connect()
loop poll loop
Exec->>Client: waitForMessage(callChannel, timeout)
alt got call
Client-->>Exec: span(call bytes)
Exec->>Handler: getFunctionArgs -> decoded args
Handler-->>Exec: sendReturnMessage(fnId, retVal)
Exec->>Client: sendMessage(returnChannel, encodedRet)
else no message / timeout
Note over Exec: idle(workCount, timeout) and continue
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Points to focus during review:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
MarioIvancik
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/bringauto/async_function_execution/clients/AeronClient.hpp (2)
109-114: Unchecked map access may dereference null publication.operator[] inserts a default‑constructed shared_ptr and
->offerwill crash if the key is unknown. Check presence.- return aeronPublications_[channelId]->offer(srcBuffer, 0, messageBytes.size()); + const auto it = aeronPublications_.find(channelId); + if (it == aeronPublications_.end() || !it->second) { + std::cerr << "Unknown or null publication for channel " << channelId << std::endl; + return -1; + } + return it->second->offer(srcBuffer, 0, messageBytes.size());
124-136: Guard missing subscription/message entries in waitForMessage.operator[] will create null entries; dereference causes UB. Validate keys and message presence.
- aeronPolling_[channelId] = true; + if (aeronSubscriptions_.find(channelId) == aeronSubscriptions_.end()) { + std::cerr << "Unknown subscription for channel " << channelId << std::endl; + return {}; + } + aeronPolling_[channelId] = true; @@ - const auto& msg = aeronMessages_[channelId]; - return std::span<const uint8_t>(msg.data, msg.length); + const auto itMsg = aeronMessages_.find(channelId); + if (itMsg == aeronMessages_.end() || itMsg->second.data == nullptr) { + return {}; + } + return std::span<const uint8_t>(itMsg->second.data, itMsg->second.length);
🧹 Nitpick comments (18)
include/bringauto/async_function_execution/Constants.hpp (1)
13-14: Consider clarifying the origin of this size limit.The value
65535(2^16 - 1) is a standard limit. If this relates to Aeron MTU, transport layer constraints, or serialization format requirements, consider adding that context to the documentation for future maintainers.examples/CMakeLists.txt (1)
7-13: Consider using PRIVATE linkage for executable targets.The three example executables link to async-function-execution-shared with PUBLIC visibility. Since these are final executables (not libraries), PRIVATE linkage is typically more appropriate and prevents unnecessary transitive dependency exposure.
Apply this diff to use PRIVATE linkage:
-TARGET_LINK_LIBRARIES(example_producer PUBLIC async-function-execution-shared) -TARGET_LINK_LIBRARIES(example_consumer PUBLIC async-function-execution-shared) -TARGET_LINK_LIBRARIES(example_driver PUBLIC async-function-execution-shared) +TARGET_LINK_LIBRARIES(example_producer PRIVATE async-function-execution-shared) +TARGET_LINK_LIBRARIES(example_consumer PRIVATE async-function-execution-shared) +TARGET_LINK_LIBRARIES(example_driver PRIVATE async-function-execution-shared)examples/main_driver.cpp (1)
7-11: Consider adding error handling and documenting shutdown.The example demonstrates minimal usage but lacks error handling for
driver.run()and provides no shutdown mechanism. For a production-like example, consider:
- Checking the return value of
driver.run()if it returns an error code- Adding signal handling (SIGINT/SIGTERM) to gracefully stop the driver
- Including comments explaining expected behavior and shutdown
Example enhancement:
+#include <csignal> + +static AeronDriver* globalDriver = nullptr; + +void signalHandler(int signal) { + if (globalDriver) { + globalDriver->stop(); + } +} + int main() { AeronDriver driver; + globalDriver = &driver; + std::signal(SIGINT, signalHandler); + std::signal(SIGTERM, signalHandler); + + std::cout << "Starting Aeron driver (press Ctrl+C to stop)..." << std::endl; driver.run(); + std::cout << "Driver stopped." << std::endl; return 0; }examples/main_producer.cpp (2)
1-6: Include what you use; avoid transitive headers.Add headers for iostream/chrono/span/string/cstdint used below.
#include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> +#include <iostream> +#include <string> +#include <span> +#include <chrono> +#include <cstdint>
40-45: Optional: demonstrate per‑function timeouts in the example config.Since the docs mention functionConfigurations, consider adding 1 example override to mirror tests.
test/include/AsyncFunctionExecutorTests.hpp (3)
6-6: Add the chrono header.You use std::chrono types below.
#include <gtest/gtest.h> +#include <chrono>
58-65: Per‑function timeouts (1–5 ms) may be flaky on CI.Nanosecond literals equate to 1–5 ms here, which is tight under load. Consider ≥50 ms unless you’re explicitly testing timeout behavior.
95-97: Assert connect() succeeds.Turn silent failures into early, actionable test errors.
- executorProducer.connect(); - executorConsumer.connect(); + ASSERT_EQ(0, executorProducer.connect()); + ASSERT_EQ(0, executorConsumer.connect());examples/README.md (1)
22-22: Add trailing newline at EOF.Minor formatting hygiene for POSIX tools.
examples/main_consumer.cpp (3)
2-2: Remove unused include.AeronDriver isn’t used here.
-#include <bringauto/async_function_execution/AeronDriver.hpp>
1-6: Include what you use.Add iostream/string/span/chrono/cstdint to avoid relying on transitive includes.
#include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> +#include <iostream> +#include <string> +#include <span> +#include <chrono> +#include <cstdint>
48-49: Check connect() result and handle failure.Prevents spinning in the poll loop after a failed init.
- executor.connect(); + const int rc = executor.connect(); + if (rc != 0) { + std::cerr << "Consumer: connect() failed, rc=" << rc << std::endl; + return rc; + }source/bringauto/async_function_execution/TimeoutIdleStrategy.cpp (2)
28-30: Reset on timeout to avoid sticky state across calls.Without reset(), subsequent idle() calls may immediately keep returning -1 due to preserved startTime_/state.
- if (std::chrono::duration_cast<std::chrono::nanoseconds>(now - startTime_) >= timeoutNs) { - return -1; - } + if (std::chrono::duration_cast<std::chrono::nanoseconds>(now - startTime_) >= timeoutNs) { + reset(); // ensure next wait starts fresh + return -1; + }
1-4: Add direct includes for used facilities.std::min, std::this_thread, and chrono are used here; include explicitly.
#include <bringauto/async_function_execution/TimeoutIdleStrategy.hpp> +#include <algorithm> // std::min +#include <thread> // std::this_thread::yield/sleep_for +#include <chrono> // std::chrono::*README.md (3)
39-40: Show connect() return handling.connect() returns int; demonstrate checking it to guide users.
-executorProducer.connect(); +if (executorProducer.connect() != 0) { + throw std::runtime_error("Failed to connect executorProducer"); +}
42-54: Clarify duration units and example.Text says “in nanoseconds” but example uses seconds. State “any std::chrono duration convertible to ns” and show a ns example.
-The functionConfigurations parameter accepts an unordered map representing per function configurations. Syntax: +functionConfigurations accepts an unordered_map of per‑function configs. Durations are any std::chrono duration convertible to nanoseconds. Syntax: @@ - - timeout: replaces the default timeout value for that function (in nanoseconds) + - timeout: replaces the default timeout for that function (stored as nanoseconds)Example:
- { 1, { std::chrono::seconds(2) } } + { 1, { std::chrono::nanoseconds{2'000'000'000} } }
24-30: Fix markdown lint: replace hard tabs and list indentation.Replace tabs with spaces and align the “Supported parameters” list to column 0 (MD010, MD007).
Also applies to: 48-49, 53-53
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
343-343: Clarify size calculation comment.The comment states "Each argument: size bytes + data", but the actual size calculation adds
2 + sizeof(args)per argument (2 bytes for the size field, plus the argument size). Consider updating the comment to explicitly mention the 2-byte size prefix for clarity.Apply this diff to clarify the comment:
- ((totalSize += 2 + sizeof(args)), ...); // Each argument: size bytes + data + ((totalSize += 2 + sizeof(args)), ...); // Each argument: 2-byte size prefix + data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CMakeLists.txt(3 hunks)README.md(2 hunks)cmake/Dependencies.cmake(0 hunks)examples/CMakeLists.txt(1 hunks)examples/README.md(1 hunks)examples/main.cpp(0 hunks)examples/main_consumer.cpp(1 hunks)examples/main_driver.cpp(1 hunks)examples/main_producer.cpp(1 hunks)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(12 hunks)include/bringauto/async_function_execution/Constants.hpp(1 hunks)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp(1 hunks)include/bringauto/async_function_execution/clients/AeronClient.hpp(5 hunks)include/bringauto/async_function_execution/clients/ClientInterface.hpp(2 hunks)include/bringauto/async_function_execution/structures/Settings.hpp(2 hunks)source/bringauto/async_function_execution/TimeoutIdleStrategy.cpp(1 hunks)test/CMakeLists.txt(1 hunks)test/README.md(1 hunks)test/include/AsyncFunctionExecutorTests.hpp(1 hunks)test/include/MockClient.hpp(1 hunks)test/integration_tests/main_segfault_consumer.cpp(2 hunks)test/integration_tests/main_segfault_producer.cpp(2 hunks)test/integration_tests/main_simple_function.cpp(2 hunks)test/source/AsyncFunctionExecutorTests.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- examples/main.cpp
- cmake/Dependencies.cmake
🧰 Additional context used
🧬 Code graph analysis (7)
examples/main_consumer.cpp (2)
examples/main_producer.cpp (3)
reinterpret_cast(11-13)bytes(14-16)bytes(14-14)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
bytes(400-407)bytes(400-400)funcId(331-337)funcId(331-331)
test/include/MockClient.hpp (3)
include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(111-114)channelId(111-111)channelId(124-136)channelId(124-124)channelId(144-163)include/bringauto/async_function_execution/clients/ClientInterface.hpp (2)
channelId(33-33)channelId(42-42)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(69-69)
include/bringauto/async_function_execution/clients/ClientInterface.hpp (3)
include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(111-114)channelId(111-111)channelId(124-136)channelId(124-124)channelId(144-163)test/include/MockClient.hpp (4)
channelId(22-78)channelId(22-22)channelId(80-88)channelId(80-80)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(69-69)
source/bringauto/async_function_execution/TimeoutIdleStrategy.cpp (1)
include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (2)
workCount(47-47)timeout(69-69)
examples/main_producer.cpp (3)
examples/main_consumer.cpp (3)
reinterpret_cast(13-15)bytes(16-18)bytes(16-16)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
bytes(400-407)bytes(400-400)test/include/MockClient.hpp (4)
bytes(113-127)bytes(113-113)bytes(130-143)bytes(130-130)
include/bringauto/async_function_execution/clients/AeronClient.hpp (3)
include/bringauto/async_function_execution/clients/ClientInterface.hpp (3)
subscriptionIds(24-24)channelId(33-33)channelId(42-42)test/include/MockClient.hpp (6)
subscriptionIds(17-20)subscriptionIds(17-17)channelId(22-78)channelId(22-22)channelId(80-88)channelId(80-80)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(69-69)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
test/include/MockClient.hpp (4)
funcId(146-153)funcId(146-146)funcId(156-167)funcId(156-156)
🪛 markdownlint-cli2 (0.18.1)
README.md
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (31)
include/bringauto/async_function_execution/Constants.hpp (4)
1-8: LGTM!Header structure is clean with appropriate include guard and minimal dependencies. The namespace declaration is correct.
9-10: Clarify the rationale for the polling limit value.The value
10appears arbitrary. Consider documenting why this specific limit was chosen (e.g., based on Aeron best practices, performance testing, or latency requirements). If this value may need tuning in different deployment scenarios, consider making it configurable rather than a hard-coded constant.
11-12: Document assumptions about function ID ranges to prevent collisions.The offset value assumes function IDs remain below a certain range to avoid collision between request and return channels. Consider documenting the expected range of function IDs (e.g., "Assumes function IDs are in range [0, 999]") or implementing validation to ensure
functionId + MESSAGE_RETURN_CHANNEL_OFFSETdoesn't collide with existing channel IDs.
16-17: LGTM!The Aeron IPC connection string is correct, and using
std::string_viewfor a compile-time string constant is appropriate. The documentation clearly indicates this is for shared memory communication.test/CMakeLists.txt (1)
6-8: LGTM! Good guard to enforce correct build workflow.The target existence check ensures tests are built as part of the main project, preventing misconfiguration. The error message is clear and actionable.
test/README.md (1)
9-14: LGTM! Documentation correctly reflects the integrated build approach.The simplified build instructions align with the CMakeLists.txt target check and make the process clearer for developers.
include/bringauto/async_function_execution/clients/ClientInterface.hpp (2)
6-6: LGTM! Include added to support timeout parameter.
39-42: All waitForMessage call sites include the timeout parameter.
No missing invocations detected across source, header, or test files.examples/CMakeLists.txt (1)
3-5: LGTM! Consistent guard with test/CMakeLists.txt.The target existence check is consistent with the test configuration and ensures examples are built correctly.
test/integration_tests/main_segfault_consumer.cpp (2)
32-34: LGTM! Simplified FunctionList initialization.The removal of the std::tuple wrapper makes the initialization cleaner and aligns with the refactored FunctionList type system.
60-60: LGTM! Added missing end-of-file newline.test/include/MockClient.hpp (1)
80-88: LGTM! Timeout parameter correctly added to match updated interface.The method signature now correctly overrides
ClientInterface::waitForMessagewith the timeout parameter. The test assertion at line 82 validates that per-function timeout values are correctly calculated based on channel ID, which is appropriate for a mock in test scenarios.test/integration_tests/main_segfault_producer.cpp (1)
33-35: LGTM! Updated FunctionList initialization aligns with API refactor.The direct braced initialization is clearer and more idiomatic than the previous tuple-based approach.
CMakeLists.txt (3)
7-7: LGTM! Version bumped to 1.0.0 for release.The major version bump appropriately signals API maturity and the timeout configuration changes introduced in this PR.
60-60: LGTM! Using ADD_SUBDIRECTORY for tests is standard practice.This provides better encapsulation and allows the test directory to manage its own CMake scope.
82-82: LGTM! Examples path normalized.Removing the trailing slash is a minor cleanup with no functional impact.
test/integration_tests/main_simple_function.cpp (2)
20-22: LGTM! Updated FunctionList initialization for producer.The direct braced initialization aligns with the API refactor and improves code clarity.
29-31: LGTM! Updated FunctionList initialization for consumer.Consistent with the producer initialization, maintaining code uniformity.
test/source/AsyncFunctionExecutorTests.cpp (1)
113-128: Connect verification already implemented: AsyncFunctionExecutorTests::SetUpTestSuite callsexecutorProducer.connect()andexecutorConsumer.connect()(lines 95–96), so the suggestion can be removed.include/bringauto/async_function_execution/clients/AeronClient.hpp (2)
124-131: UseTimeoutIdleStrategyas the default IdleStrategy
AeronClient.hpp currently defaults toaeron::BackoffIdleStrategy, which does not support the callidle(fragmentsRead, timeout). Change the template default toTimeoutIdleStrategyor introduce an adapter that provides bothidle(int, timeout)andidle(int)overloads to avoid compilation errors.
152-155: idle(fragmentsRead) already respects the configured timeout – it calls idle(fragmentsRead, std::chrono::nanoseconds{0}), which falls back to the default timeout in TimeoutIdleStrategy; no change needed.Likely an incorrect or invalid review comment.
include/bringauto/async_function_execution/structures/Settings.hpp (2)
18-26: LGTM! Clean refactor to structured configuration.The introduction of
FunctionConfigsas a typed wrapper around the configuration map is a solid improvement over JSON string parsing. The move semantics in the constructor are correctly applied.
38-42: LGTM! Constructor signature is clear and idiomatic.The updated
Settingsconstructor correctly acceptsFunctionConfigsby const reference with a sensible default, maintaining good API ergonomics.include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (3)
16-37: LGTM! Constructor documentation is clear.The updated documentation accurately describes the constructor parameters and their defaults.
39-52: LGTM! Public API is well-designed.The new
idle(int workCount, std::chrono::nanoseconds timeout)signature provides a clean interface for timeout-aware idling, and thereset()method allows state reinitialization.
54-69: Good encapsulation by moving IdleState to private scope.Moving the
IdleStateenum to the private section is a positive change that reduces the public API surface and prevents external dependencies on internal state representation.include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (5)
22-22: LGTM! Consistent with Settings.hpp refactor.The change to
structures::FunctionConfigsaligns with the structured configuration approach introduced in Settings.hpp.
89-126: LGTM! Well-designed type traits and deduction guide.The
is_function_definitiontrait,IsFunctionDefinitionconcept, and the deduction guide forFunctionListprovide strong compile-time guarantees and improve API usability.
143-161: LGTM! Constructor validation improves robustness.The constructor correctly validates that all function IDs in the configuration are defined in the FunctionList, catching configuration errors early. The optional client parameter adds useful flexibility for testing and custom implementations.
172-185: LGTM! Return type change improves error handling.Changing
connect()to returnintallows callers to detect connection failures, which is a significant improvement over the previous void return.
212-216: Zero timeout yields infinite wait (no fallback to defaultTimeout). Verified that whenFunctionConfig.timeoutis zero,waitForMessagepasses nanoseconds(0) toTimeoutIdleStrategy, which skips the timeout check and idles indefinitely (per its docs and implementation).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/integration_tests/main_simple_function.cpp (1)
1-6: Include what you use: add and .This TU uses std::thread and std::this_thread::sleep_for but doesn’t include their headers. Add explicit includes to avoid transitive‑include breakage.
#include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> #include <bringauto/async_function_execution/AeronDriver.hpp> #include <iostream> +#include <thread> +#include <chrono>test/integration_tests/main_segfault_producer.cpp (1)
1-6: Include what you use: add and .This TU launches a std::thread and sleeps.
#include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> #include <bringauto/async_function_execution/AeronDriver.hpp> #include <iostream> +#include <thread> +#include <chrono> namespace baafe = bringauto::async_function_execution;Also applies to: 9-14
include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
19-20: Default IdleStrategy mismatches idle(fragmentsRead, timeout); set to TimeoutIdleStrategy.BackoffIdleStrategy typically lacks idle(int, std::chrono::nanoseconds) and returns void. Using it as default can cause compile errors if instantiated. Make TimeoutIdleStrategy the default.
-template<typename IdleStrategy = aeron::BackoffIdleStrategy> +template<typename IdleStrategy = bringauto::async_function_execution::TimeoutIdleStrategy>include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
143-161: Verify function config validation logic.Lines 156-160 validate that all function IDs in the configuration are defined in the FunctionList. However, the error message (line 158) says "Warning:" but then throws an exception, which is contradictory.
Apply this diff to make the error message consistent:
- throw std::runtime_error("Warning: Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList."); + throw std::runtime_error("Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList.");
♻️ Duplicate comments (2)
examples/main_consumer.cpp (1)
58-73: Return type fixed to SerializableString.Calls to sendReturnMessage now pass SerializableString as required. Resolves prior review.
include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
69-99: Bounded waits in connect() are a solid improvement.Timeouts with early returns and logging address the previous busy‑wait risk.
🧹 Nitpick comments (10)
README.md (1)
15-31: Replace hard tabs with spaces to satisfy markdownlint.The new blocks introduced literal tab characters and a mis-indented bullet, triggering MD010/MD007 from markdownlint-cli2. Swap tabs for spaces (e.g., four spaces) and align the bullet list so the pipeline stays green.
Also applies to: 49-55
include/bringauto/async_function_execution/structures/Settings.hpp (1)
23-26: Include<utility>forstd::move.This header now calls
std::move, but<utility>isn’t included. Pull it in here to avoid depending on indirect includes.test/include/AsyncFunctionExecutorTests.hpp (1)
94-97: Assert connect() success in SetUpTestSuite.connect() now returns int. Assert/FAIL on non‑zero to avoid hidden failures.
static void SetUpTestSuite() { - executorProducer.connect(); - executorConsumer.connect(); + ASSERT_EQ(0, executorProducer.connect()) << "Producer connect failed"; + ASSERT_EQ(0, executorConsumer.connect()) << "Consumer connect failed"; }test/source/AsyncFunctionExecutorTests.cpp (1)
134-138: ArgumentTooLarge test adds useful boundary coverage.Consider adding a 65535‑byte success case to pin the exact limit.
include/bringauto/async_function_execution/clients/AeronClient.hpp (4)
171-174: Verify IdleStrategy interface consistency in waitForAnyMessage.Here idle(fragmentsRead) is called (1 arg), but waitForMessage calls idle(fragmentsRead, timeout). Ensure the chosen IdleStrategy supports both or refactor to a single consistent signature.
74-82: Improve log context for timeouts.Include pub/sub IDs to aid diagnosis.
- std::cerr << "Aeron connection error: Timeout while waiting for publication" << std::endl; + std::cerr << "Aeron connection error: Timeout while waiting for publication id=" << pubId << std::endl; @@ - std::cerr << "Aeron connection error: Timeout while waiting for subscription" << std::endl; + std::cerr << "Aeron connection error: Timeout while waiting for subscription id=" << subId << std::endl;Also applies to: 89-97
121-129: Minor: avoid const_cast by constructing AtomicBuffer from a mutable copy.Not critical, but reduces UB risk if the impl writes. Optional.
65-110: Optional: brief sleep in connect loops to reduce CPU.Yield-only loops can still spin. A tiny sleep (e.g., 1ms) reduces load without hurting latency.
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
401-407: Redundant empty check.Line 401 checks if
bytes.empty()before accessingbytes[0]on line 404. However, this check is redundant because:
- In
pollFunction()(line 244), an empty span is already returned ifrequestBytes.empty()- The check adds minimal value since callers should validate before calling this private method
Consider removing the redundant check or documenting why it's defensive:
std::tuple<FunctionId, std::span<const uint8_t>> deserializeRequest(const std::span<const uint8_t>& bytes) { - if (bytes.empty()) { - throw std::invalid_argument("Not enough data to deserialize request"); - } + // bytes guaranteed non-empty by pollFunction() FunctionId funcId{ bytes[0] }; std::span<const uint8_t> args(bytes.begin() + 1, bytes.end()); return std::make_tuple(funcId, args); }test/include/MockClient.hpp (1)
80-88: Derive expected timeout from test configuration instead of hardcoding formula
In MockClient::waitForMessage (test/include/MockClient.hpp:80–82), replace the literal(channelId - 1000) * 1'000'000with a lookup of the configured timeout for functionId = channelId - 1000. This ensures the EXPECT_EQ compares against the actual test setup and remains correct if timeouts change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.md(2 hunks)examples/README.md(1 hunks)examples/main_consumer.cpp(1 hunks)examples/main_producer.cpp(1 hunks)include/bringauto/async_function_execution/AeronDriver.hpp(1 hunks)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(17 hunks)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp(1 hunks)include/bringauto/async_function_execution/clients/AeronClient.hpp(4 hunks)include/bringauto/async_function_execution/clients/ClientInterface.hpp(2 hunks)include/bringauto/async_function_execution/structures/Settings.hpp(2 hunks)source/bringauto/async_function_execution/AeronDriver.cpp(2 hunks)source/bringauto/async_function_execution/TimeoutIdleStrategy.cpp(1 hunks)test/README.md(1 hunks)test/include/AsyncFunctionExecutorTests.hpp(2 hunks)test/include/MockClient.hpp(6 hunks)test/integration_tests/main_segfault_consumer.cpp(3 hunks)test/integration_tests/main_segfault_producer.cpp(4 hunks)test/integration_tests/main_simple_function.cpp(3 hunks)test/source/AsyncFunctionExecutorTests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/main_producer.cpp
- source/bringauto/async_function_execution/TimeoutIdleStrategy.cpp
🧰 Additional context used
🧬 Code graph analysis (8)
test/integration_tests/main_simple_function.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(94-97)
include/bringauto/async_function_execution/clients/ClientInterface.hpp (3)
include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(121-129)channelId(121-121)channelId(139-156)channelId(139-139)channelId(164-183)test/include/MockClient.hpp (4)
channelId(22-78)channelId(22-22)channelId(80-88)channelId(80-80)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(64-64)
test/source/AsyncFunctionExecutorTests.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(94-97)
test/include/MockClient.hpp (4)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
funcId(331-337)funcId(331-331)bytes(400-407)bytes(400-400)include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(121-129)channelId(121-121)channelId(139-156)channelId(139-139)channelId(164-183)include/bringauto/async_function_execution/clients/ClientInterface.hpp (2)
channelId(32-32)channelId(41-41)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(64-64)
test/integration_tests/main_segfault_producer.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(94-97)
include/bringauto/async_function_execution/clients/AeronClient.hpp (3)
include/bringauto/async_function_execution/clients/ClientInterface.hpp (3)
subscriptionIds(23-23)channelId(32-32)channelId(41-41)test/include/MockClient.hpp (6)
subscriptionIds(17-20)subscriptionIds(17-17)channelId(22-78)channelId(22-22)channelId(80-88)channelId(80-80)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(64-64)
examples/main_consumer.cpp (2)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
bytes(400-407)bytes(400-400)funcId(331-337)funcId(331-331)test/include/MockClient.hpp (8)
bytes(113-127)bytes(113-113)bytes(130-143)bytes(130-130)funcId(146-153)funcId(146-146)funcId(156-167)funcId(156-156)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
test/include/MockClient.hpp (8)
funcId(146-153)funcId(146-146)funcId(156-167)funcId(156-156)bytes(113-127)bytes(113-113)bytes(130-143)bytes(130-130)include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
buffer(186-191)
🪛 markdownlint-cli2 (0.18.1)
README.md
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (35)
source/bringauto/async_function_execution/AeronDriver.cpp (3)
3-3: Good modernization: C++ header preferred.Replacing
<signal.h>with<csignal>aligns with modern C++ best practices.
29-29: Good modernization: nullptr is type-safe.Using
nullptrinstead ofNULLis a C++11 best practice that provides better type safety.
37-42: Const-correctness applied logically.Marking
run()asconstis technically valid since it doesn't modify the observable state of theAeronDriverwrapper object itself. The underlyingaeron_driver_tstate modifications are external to this wrapper's members.This design treats the wrapper as a handle to the driver rather than the driver itself—a reasonable approach for RAII/wrapper classes.
include/bringauto/async_function_execution/AeronDriver.hpp (2)
27-27: Declaration matches implementation.The
constqualifier aligns with the implementation inAeronDriver.cppand correctly reflects that this method doesn't modify the wrapper object's observable state.
33-33: [[nodiscard]] appropriately applied.Adding
[[nodiscard]]toisRunning()is correct since this is a pure query method with no side effects. Ignoring its return value would be a logic error that this attribute helps catch at compile time.test/integration_tests/main_simple_function.cpp (2)
20-23: FunctionList brace-init looks good.Also applies to: 29-32
65-72: Good: handle connect() failures.Checking the int return and failing fast improves test reliability.
test/include/AsyncFunctionExecutorTests.hpp (3)
15-15: Explicit SerializableString ctor is appropriate.
58-66: Typed per‑function timeouts wired correctly.Config uses FunctionConfigs with nanoseconds per ID; consistent with MockClient timeout checks.
67-74: FunctionList brace-init update is fine.test/integration_tests/main_segfault_consumer.cpp (1)
39-43: Good: handle connect() failure.test/integration_tests/main_segfault_producer.cpp (2)
54-57: Good: connect() error handling.
64-65: Good: pass SerializableString to callFunc.test/source/AsyncFunctionExecutorTests.cpp (2)
118-126: Undefined function config test updated correctly.
30-38: Potential flakiness: PollFunction has no arranged request.If no producer request is sent before this test, pollFunction may block or return empty. Ensure the test sends a request (or uses a mock setup) within this test.
include/bringauto/async_function_execution/clients/AeronClient.hpp (2)
139-151: Channel existence checks and timeout-aware idle are good.Nice guard on missing channel and timeout return path.
1-207: Ignore unintended default‐instantiation warning: allAeronClientusages (e.g., inAsyncFunctionExecutor) explicitly specify<TimeoutIdleStrategy>, so no default template argument is ever used.Likely an incorrect or invalid review comment.
test/include/MockClient.hpp (7)
12-12: LGTM!Marking the mock class as
finalis appropriate and enables compiler optimizations since this is a leaf test class with no intended inheritance.
45-78: Const-correctness improvements look good.The addition of
constqualifiers to local variables (funcId,args,sum,product) improves code clarity and prevents accidental mutations. The logic remains unchanged and correct.
113-127: Const-correctness improvements look good.The parameter is now
const std::span<const uint8_t> &bytes(line 113), and local variablesargCount(line 115) andargSize(line 119) are markedconst. This prevents accidental mutations and improves readability.
130-143: Const-correctness improvements look good.Similar to
deserializeIntRequest, the parameter and local variables (argCountat line 132,argSizeat line 136) are now properly const-qualified.
146-153: Const-correctness improvements look good.The parameters
funcIdandreturnValueare now const-qualified (line 146), and the local variable at line 149 is markedconst. The serialization logic remains correct.
156-167: Const-correctness improvements look good.The parameters are const-qualified (line 156), and local size calculation (lines 162-164) is properly const. The validation at lines 159-161 correctly prevents oversized strings from being serialized.
91-109: No action needed: size encoding matches AsyncFunctionExecutor’s little-endian 16-bit format.include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (11)
22-23: LGTM!The Config structure now uses
structures::FunctionConfigsinstead of a raw type, providing better type safety and structure for per-function timeout configurations.
28-32: Document supported range for FunctionId.Line 28 documents that the supported range is 0-255, which is appropriate given
valueisuint8_t. This is a helpful clarification for users.
46-72: Explicit constructors improve type safety.The
Return<T>(line 51),Return<void>(line 60), andArguments<Args...>(line 71) constructors are nowexplicit, preventing implicit conversions and improving type safety. The addedvalueandvaluesmembers (lines 50, 70) make the structures more self-documenting.
77-127: Strong typing for FunctionDefinition and FunctionList.The additions provide compile-time guarantees:
- Lines 89-108: Type trait
is_function_definitionand conceptIsFunctionDefinitionensure only validFunctionDefinitiontypes are used- Lines 115-119:
FunctionListis now constrained byIsFunctionDefinitionand uses an explicit constructor- Lines 122-126: Deduction guide simplifies construction
These changes strengthen the type system and improve compile-time safety.
176-179: Const-correctness in lambda captures looks good.Lines 176 and 179 now use
const auto&in lambda captures withinstd::apply, preventing unnecessary copies and accidental mutations.
212-216: Per-function timeout lookup is correct.Lines 212-216 implement per-function timeout resolution:
- First checks if the function ID exists in
functionConfigs.configs- Falls back to
defaultTimeoutif not found- Uses the ternary operator for concise selection
The logic is correct and handles the fallback case properly.
243-250: Const-correctness improvements look good.Line 243 adds
consttorequestBytes, preventing accidental modification. The rest of the method remains unchanged.
271-303: Const-correctness and validation improvements.Line 271 adds an
empty()check before accessingargBytes[pos], preventing potential out-of-bounds access. Line 285 marks the length variableconst. These changes improve safety and code clarity.
333-337: Const-correctness in lambda captures looks good.Lines 333 and 335 use
const auto&in lambda captures, consistent with the changes at lines 176-179.
343-381: Const-correctness in serialization helpers looks good.Lines 343, 355, 370, 372, 376, and 378 add
constqualifiers to size calculations and intermediate variables. This prevents accidental mutations and improves code clarity without changing behavior.
412-412: Documentation improvement acknowledged.The comment clarifies that
client_can be a customClientInterfaceimplementation, which is helpful for users extending the library.
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/main_producer.cpp (1)
52-57: Consider demonstrating error handling for callFunc().The example calls
.value()directly on thestd::expectedreturn, which will throw on error. Since this is a producer example demonstrating the API, consider showing the recommended error-handling pattern:auto result1 = executor.callFunc(ExampleFunc1, 42, "Hello", 3.14f); if (!result1.has_value()) { std::cerr << "ExampleFunc1 failed with error" << std::endl; return 1; } std::cout << result1.value().value << std::endl;This would help users understand how to handle errors gracefully.
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
149-175: Consider updating the error message consistency.The error message on line 172 starts with "Warning:" but immediately throws a
std::runtime_error. Consider changing "Warning:" to "Error:" for consistency since this is a fatal error that prevents construction.- throw std::runtime_error("Warning: Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList."); + throw std::runtime_error("Error: Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList.");Otherwise, the validation logic is sound and provides good protection against configuration errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(3 hunks)examples/CMakeLists.txt(1 hunks)examples/main_producer.cpp(1 hunks)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(17 hunks)test/CMakeLists.txt(1 hunks)test/include/AsyncFunctionExecutorTests.hpp(2 hunks)test/include/MockClient.hpp(6 hunks)test/integration_tests/main_segfault_producer.cpp(4 hunks)test/integration_tests/main_simple_function.cpp(3 hunks)test/source/AsyncFunctionExecutorTests.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CMakeLists.txt
- test/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T13:53:35.438Z
Learnt from: MarioIvancik
PR: bringauto/async-function-execution#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:
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpptest/include/MockClient.hpp
🧬 Code graph analysis (6)
test/integration_tests/main_simple_function.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(102-105)
examples/main_producer.cpp (2)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
bytes(420-427)bytes(420-420)test/include/MockClient.hpp (4)
bytes(119-133)bytes(119-119)bytes(136-149)bytes(136-136)
test/source/AsyncFunctionExecutorTests.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(102-105)
test/integration_tests/main_segfault_producer.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(102-105)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
test/include/MockClient.hpp (10)
funcId(152-159)funcId(152-152)funcId(162-173)funcId(162-162)funcId(176-180)funcId(176-176)bytes(119-133)bytes(119-119)bytes(136-149)bytes(136-136)include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
buffer(186-191)
test/include/MockClient.hpp (3)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
funcId(347-353)funcId(347-347)bytes(420-427)bytes(420-420)include/bringauto/async_function_execution/clients/ClientInterface.hpp (2)
channelId(32-32)channelId(41-41)include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(121-129)channelId(121-121)channelId(139-156)channelId(139-139)channelId(164-183)
🔇 Additional comments (39)
examples/main_producer.cpp (3)
6-17: LGTM!The SerializableString implementation is straightforward and correct for the example's purpose. The serialize() method returns a span pointing to the internal string data, which is safe given the usage pattern in this example.
19-35: LGTM!The function definitions are well-structured and demonstrate the variety of argument types supported by the AsyncFunctionExecutor.
39-45: LGTM!The AsyncFunctionExecutor construction is correct and follows the new configuration pattern with per-function capabilities.
test/include/MockClient.hpp (6)
12-12: LGTM!Marking the test mock as
finalis good practice and prevents unintended inheritance.
86-94: LGTM!The timeout verification logic correctly validates that per-function timeouts are properly propagated to the client layer. The formula
(channelId - 1000) * 1000000aligns with the test configuration in AsyncFunctionExecutorTests.hpp.
55-59: LGTM!The FunctionWait handler correctly demonstrates void return type handling and simulates asynchronous work with a sleep. This aligns with the new FunctionWait definition in the test suite.
119-149: LGTM!The const-correctness improvements to the deserialization methods are well done. Using
constqualifiers consistently improves code clarity and prevents accidental modifications.
152-173: LGTM!The serialization methods now use const parameters consistently, which improves API clarity and safety.
175-180: LGTM!The new
serializeVoidResponsemethod correctly handles void return types by serializing only the function ID. This is essential for testing functions with void returns.include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (14)
20-24: LGTM!The Config struct now uses a typed
FunctionConfigsstructure instead of string-based configuration, improving type safety and API clarity.
28-33: LGTM!The documentation update clarifies the supported function ID range (0-255), which aligns with the
uint8_tstorage type.
49-62: LGTM!The Return constructors are well-designed with explicit keyword to prevent implicit conversions, perfect forwarding for efficiency, and constexpr for compile-time evaluation where possible.
69-73: LGTM!The Arguments constructor follows the same good design principles as Return, using explicit, constexpr, and perfect forwarding.
90-109: LGTM!The type trait and concept provide compile-time validation that FunctionList only contains valid FunctionDefinition types, improving type safety.
112-127: LGTM!The FunctionList design with concept constraints and deduction guide provides a clean, type-safe API for defining function lists.
130-140: LGTM!The CallError enum provides comprehensive error states for function calls, enabling proper error handling and diagnostics.
180-200: LGTM!The connect() method correctly initializes the in-progress tracking map and returns the connection status. The use of
std::applywith a lambda to populate the channel vectors and in-progress map is elegant.
203-244: LGTM!The callFunc() method is well-implemented with proper error handling, in-progress tracking, and per-function timeout support. The validation checks cover all error cases, and the in-progress flag is properly managed on both success and error paths.
247-266: LGTM!The pollFunction() method correctly retrieves incoming function calls and deserializes the request into function ID and arguments.
269-319: LGTM!The getFunctionArgs() method has proper validation, const-correctness, and handles both serializable and trivially-copyable types correctly.
322-344: LGTM!The sendReturnMessage() method has appropriate validation and returns proper error codes.
400-417: LGTM!The deserializeReturn() method correctly handles both void and non-void return types using if constexpr, and properly returns std::expected for error handling.
430-437: LGTM!The in-progress tracking map using
std::atomic_boolprovides thread-safe tracking of concurrent function calls, preventing race conditions.test/integration_tests/main_simple_function.cpp (3)
15-32: LGTM!The FunctionList initialization uses the new direct braced form, which is clean and leverages the deduction guide.
65-72: LGTM!The connect() calls now have proper error handling with descriptive error messages and early returns on failure.
80-80: LGTM!Using
.value()directly in an integration test is acceptable, as any error will cause the test to fail appropriately.test/include/AsyncFunctionExecutorTests.hpp (4)
12-23: LGTM!Making the SerializableString constructor explicit prevents implicit conversions and improves type safety in the tests.
55-59: LGTM!The FunctionWait definition is useful for testing void return types and function call in-progress tracking.
61-83: LGTM!The per-function timeout configuration uses the new typed FunctionConfigs structure and provides good test coverage with distinct timeouts for each function. The timeout pattern (funcId * 1000000 ns) aligns with the MockClient's verification logic.
85-97: LGTM!The consumer configuration is appropriately simplified, and the FunctionList correctly excludes FunctionWait since it's only needed for producer tests.
test/integration_tests/main_segfault_producer.cpp (4)
9-21: LGTM!The explicit constructor for SerializableString is consistent with other test files and improves type safety.
29-37: LGTM!The FunctionList initialization uses the new direct braced form correctly.
54-57: LGTM!The connect() error handling is properly implemented with descriptive error messages.
64-64: LGTM!The callFunc call now uses explicit SerializableString construction, which is required due to the explicit constructor. Using
.value()is acceptable in integration tests.test/source/AsyncFunctionExecutorTests.cpp (5)
5-24: LGTM!The tests correctly use
.value()to unwrapstd::expectedresults and use explicit SerializableString construction where needed.
72-107: LGTM!The tests now use the
std::expectedAPI correctly, checking.error()for expected error cases instead of catching exceptions. This is cleaner and more explicit.
110-128: LGTM!The test correctly verifies that providing configuration for undefined function IDs throws an error, using the new typed FunctionConfigs structure.
131-138: LGTM!The ArgumentTooLarge test correctly validates that arguments exceeding the 2^16 byte limit are properly rejected with
std::invalid_argument.
141-155: LGTM!The FunctionCallInProgress test correctly validates that concurrent calls to the same function are prevented by the in-progress tracking mechanism.
Note: This test relies on timing (50ms sleep) to catch the in-progress state. While the 200ms function execution time in MockClient provides margin, timing-based tests can occasionally be flaky in heavily loaded CI environments.
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
Outdated
Show resolved
Hide resolved
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
Outdated
Show resolved
Hide resolved
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
Outdated
Show resolved
Hide resolved
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
Outdated
Show resolved
Hide resolved
include/bringauto/async_function_execution/clients/AeronClient.hpp
Outdated
Show resolved
Hide resolved
include/bringauto/async_function_execution/structures/Settings.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
347-357: Incorrect size calculation for types with serialize().Line 351 uses
sizeof(args)to estimate buffer size, but if an argument has aserialize()method, its actual serialized size may differ fromsizeof. This causes over- or under-reservation inserializationBuffer_.While
reserve()is just a performance hint and won't cause correctness issues, accurate sizing avoids unnecessary reallocations. To fix this, you'd need to callserialize()on each argument conditionally, which complicates the code. Consider whether accurate pre-sizing is worth the complexity, or document that the reserve is a heuristic.The same issue exists at Line 363 in
serializeReturn.
♻️ Duplicate comments (1)
examples/main_consumer.cpp (1)
21-23: Avoid hardcoded function ID constants.The function definitions already provide IDs via their
id.valuemembers. Using the constants in switch cases creates maintenance burden if IDs change.Based on learnings
Remove these constants and use
ExampleFunc1.id.value,ExampleFunc2.id.value, andExampleFunc3.id.valuedirectly in the switch cases (lines 61, 67, 73).
🧹 Nitpick comments (5)
include/bringauto/async_function_execution/structures/Settings.hpp (2)
26-31: Optimize getConfig to avoid double lookup.The method calls both
find()andat(), performing two map lookups when one suffices.Apply this diff:
FunctionConfig getConfig(const uint8_t functionId) const { - if (configs.find(functionId) != configs.end()) { - return configs.at(functionId); + auto it = configs.find(functionId); + if (it != configs.end()) { + return it->second; } return FunctionConfig{}; }
37-43: Pre-allocate vector capacity in getFunctionIds.Reserving capacity upfront avoids multiple reallocations.
Apply this diff:
std::vector<uint8_t> getFunctionIds() const { std::vector<uint8_t> functionIds; + functionIds.reserve(configs.size()); for (const auto& [funcId, _] : configs) { functionIds.push_back(funcId); } return functionIds; }examples/main_consumer.cpp (1)
57-83: Handle empty function ID from pollFunction.When
pollFunction()times out or fails, it returns an emptyFunctionId{}(value 0). The switch doesn't guard against this, so value 0 would hit the default case and throw, which may not be the desired behavior for a timeout.Add a check before the switch:
while (true) { auto [funcId, argBytes] = executor.pollFunction(); + + if (funcId.value == 0 && argBytes.empty()) { + continue; // Timeout or no message, retry + } switch (funcId.value) {include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
163-167: Remove "Warning:" prefix from exception message.The message begins with "Warning:" but immediately throws, making it an error rather than a warning.
Apply this diff:
if (!isFunctionDefined(FunctionId{funcId})) { - throw std::runtime_error("Warning: Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList."); + throw std::runtime_error("Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList"); }
339-345: Simplify fold expression for clarity.The ternary operator in Line 342 is correct but potentially confusing. The
falsebranch doesn't modifyfound, relying on the reader to understand ternary semantics.Use a clearer idiom:
bool isFunctionDefined(const FunctionId &funcId) { bool found = false; std::apply([&](const auto&... funcDefs) { - ((funcDefs.id.value == funcId.value ? found = true : false), ...); + ((found = found || (funcDefs.id.value == funcId.value)), ...); }, functions_.functions); return found; }Or more concisely:
bool isFunctionDefined(const FunctionId &funcId) { bool found = false; std::apply([&](const auto&... funcDefs) { - ((funcDefs.id.value == funcId.value ? found = true : false), ...); + (((funcDefs.id.value == funcId.value) && (found = true)), ...); }, functions_.functions); return found; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/main_consumer.cpp(1 hunks)include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(17 hunks)include/bringauto/async_function_execution/Constants.hpp(1 hunks)include/bringauto/async_function_execution/clients/AeronClient.hpp(4 hunks)include/bringauto/async_function_execution/structures/Settings.hpp(2 hunks)test/include/AsyncFunctionExecutorTests.hpp(2 hunks)test/include/MockClient.hpp(6 hunks)test/source/AsyncFunctionExecutorTests.cpp(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
test/include/AsyncFunctionExecutorTests.hppinclude/bringauto/async_function_execution/clients/AeronClient.hppexamples/main_consumer.cpptest/source/AsyncFunctionExecutorTests.cppinclude/bringauto/async_function_execution/Constants.hppinclude/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 Learning: 2025-10-02T13:53:35.438Z
Learnt from: MarioIvancik
PR: bringauto/async-function-execution#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/include/AsyncFunctionExecutorTests.hppinclude/bringauto/async_function_execution/clients/AeronClient.hpptest/include/MockClient.hpptest/source/AsyncFunctionExecutorTests.cppinclude/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 Learning: 2024-10-22T11:03:16.796Z
Learnt from: MarioIvancik
PR: bringauto/fleet-protocol-http-client-cxx#13
File: example/main.cpp:30-32
Timestamp: 2024-10-22T11:03:16.796Z
Learning: In `example/main.cpp`, the code serves as an example implementation and may not require detailed error messaging enhancements.
Applied to files:
examples/main_consumer.cpp
📚 Learning: 2024-10-22T11:03:33.918Z
Learnt from: MarioIvancik
PR: bringauto/fleet-protocol-http-client-cxx#13
File: example/main.cpp:29-37
Timestamp: 2024-10-22T11:03:33.918Z
Learning: In the C++ project, `example/main.cpp` is an example implementation and may not require refactoring or code optimization during code reviews.
Applied to files:
examples/main_consumer.cpp
🧬 Code graph analysis (5)
include/bringauto/async_function_execution/clients/AeronClient.hpp (3)
test/include/MockClient.hpp (6)
subscriptionIds(17-20)subscriptionIds(17-17)channelId(22-85)channelId(22-22)channelId(87-95)channelId(87-87)include/bringauto/async_function_execution/clients/ClientInterface.hpp (3)
subscriptionIds(23-23)channelId(32-32)channelId(41-41)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(64-64)
test/include/MockClient.hpp (3)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
funcId(339-345)funcId(339-339)bytes(412-419)bytes(412-412)include/bringauto/async_function_execution/clients/AeronClient.hpp (5)
channelId(121-129)channelId(121-121)channelId(139-156)channelId(139-139)channelId(164-183)include/bringauto/async_function_execution/clients/ClientInterface.hpp (2)
channelId(32-32)channelId(41-41)
examples/main_consumer.cpp (2)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (4)
bytes(412-419)bytes(412-412)funcId(339-345)funcId(339-339)test/include/MockClient.hpp (10)
bytes(120-134)bytes(120-120)bytes(137-150)bytes(137-137)funcId(153-160)funcId(153-153)funcId(163-174)funcId(163-163)funcId(177-181)funcId(177-177)
test/source/AsyncFunctionExecutorTests.cpp (1)
test/include/AsyncFunctionExecutorTests.hpp (1)
executorProducer(102-105)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (3)
test/include/MockClient.hpp (10)
funcId(153-160)funcId(153-153)funcId(163-174)funcId(163-163)funcId(177-181)funcId(177-177)bytes(120-134)bytes(120-120)bytes(137-150)bytes(137-137)include/bringauto/async_function_execution/TimeoutIdleStrategy.hpp (1)
timeout(64-64)include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
buffer(186-191)
🪛 Clang (14.0.6)
examples/main_consumer.cpp
[error] 1-1: 'bringauto/async_function_execution/AsyncFunctionExecutor.hpp' file not found
(clang-diagnostic-error)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (1)
124-133: Consider expanding enum documentation.The inline comments are helpful, but each error case could benefit from more detailed documentation explaining:
- When the error occurs
- How to resolve it
- Example scenarios
This would improve developer experience when handling these errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(17 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-03T10:39:48.715Z
Learnt from: MarioIvancik
Repo: bringauto/async-function-execution PR: 3
File: include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp:214-244
Timestamp: 2025-11-03T10:39:48.715Z
Learning: In the bringauto/async-function-execution repository, exceptions thrown from AsyncFunctionExecutor::callFunc() (specifically from client_->sendMessage(), client_->waitForMessage(), or deserializeReturn()) are considered fatal errors indicating unrecoverable infrastructure failures. The program is expected to terminate in such cases, so exception safety cleanup (like resetting callInProgress_ flags) is intentionally not implemented.
Applied to files:
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 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/async_function_execution/AsyncFunctionExecutor.hpp
📚 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:
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 Learning: 2024-10-22T11:03:16.796Z
Learnt from: MarioIvancik
Repo: bringauto/fleet-protocol-http-client-cxx PR: 13
File: example/main.cpp:30-32
Timestamp: 2024-10-22T11:03:16.796Z
Learning: In `example/main.cpp`, the code serves as an example implementation and may not require detailed error messaging enhancements.
Applied to files:
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
🧬 Code graph analysis (1)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
test/include/MockClient.hpp (10)
funcId(153-160)funcId(153-153)funcId(163-174)funcId(163-163)funcId(177-181)funcId(177-177)bytes(120-134)bytes(120-120)bytes(137-150)bytes(137-137)include/bringauto/async_function_execution/structures/Settings.hpp (4)
functionId(26-31)functionId(26-26)functionId(33-35)functionId(33-33)
🔇 Additional comments (12)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (12)
20-24: LGTM! Typed configuration improves type safety.The change from
std::string_view(JSON-based) tostructures::FunctionConfigsaligns with the PR's objective to use typed structures and eliminates runtime JSON parsing errors.
93-109: LGTM! Strong type constraints improve compile-time safety.The
is_function_definitiontrait andIsFunctionDefinitionconcept provide robust compile-time validation thatFunctionListonly acceptsFunctionDefinitiontypes.
116-120: LGTM! Explicit constructor prevents implicit conversions.The
IsFunctionDefinitionconstraint and explicit constructor are appropriate defensive measures.
214-244: LGTM! Good use of std::expected for error handling.The function properly validates inputs, tracks call-in-progress state, and uses
std::expectedfor clear error propagation. The per-function timeout configuration is a nice addition.Note: As per learnings, exceptions from
sendMessage(),waitForMessage(), ordeserializeReturn()indicate fatal infrastructure failures, so thecallInProgress_flag is intentionally not reset in exception paths.Based on learnings.
253-266: LGTM! Updated to use waitForAnyMessage().The change from
waitForMessage()towaitForAnyMessage()is appropriate for consumer-mode polling where the executor should receive any incoming function call.
287-287: LGTM! More idiomatic empty check.Using
argBytes.empty()is clearer thanargBytes.size() < 1.
330-344: LGTM! Consistent channelOffset handling.The method correctly applies
channelOffset_to the return channel ID, maintaining consistency with the producer-side expectations.
347-353: LGTM! Appropriate use of const reference.Changing from
auto&&...toconst auto&...is correct since the lambda only reads the function definitions without modifying them.
356-365: Fold expression evaluation order is deterministic.Line 359 uses a fold expression
((totalSize += 2 + sizeof(args)), ...)which evaluates left-to-right. This is well-defined in C++17 and later.However, note that a past reviewer (koudis) raised a concern about "order of argument evaluation" being "dangerous" and potentially "random". If this refers to a different line or aspect of the code, please clarify.
401-417: LGTM! Proper std::expected usage for void and non-void returns.The method correctly handles both void and non-void return types with
std::expected, providing clear error propagation for function ID mismatches.
420-440: LGTM! Thread-safe call tracking with atomic_bool.The use of
std::atomic_boolincallInProgress_(line 437) ensures thread-safe concurrent access to the in-progress flags. The added documentation for private members is also helpful.
10-10: No issues found. C++23 standard is properly configured.The codebase is set to use C++23 (CMAKE_CXX_STANDARD is set to 23 in CMakeLists.txt), so the inclusion of
<expected>is valid and properly supported.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (2)
163-167: Consider validating in reverse: iterate FunctionList to check configs.Currently, the code iterates
functionConfigs.getFunctionIds()and validates each is defined. Per review feedback, consider iterating theFunctionListinstead and validating that required functions have configurations, which would make the validation direction match the primary source of truth (the FunctionList).Apply this diff to iterate FunctionList:
- for (const auto &funcId: settings_.functionConfigs.getFunctionIds()) { - if (!isFunctionDefined(FunctionId{funcId})) { - throw std::runtime_error("Warning: Function ID " + std::to_string(funcId) + " in configuration is not defined in FunctionList."); - } - } + std::apply([&](const auto&... funcDefs) { + // Validation logic here if needed for each function definition + ((void)funcDefs, ...); + }, functions_.functions);Note: Since
FunctionConfigs::getConfig()returns a defaultFunctionConfig{}for missing entries, explicit validation may not be necessary unless you want to warn about orphaned config entries.
386-394: Consider explicit parentheses in bitwise operations for clarity.The expressions
size >> 8 & 0xFFat lines 388 and 394 are correct due to operator precedence (>>binds before&), but adding explicit parentheses improves readability:- buffer.push_back(static_cast<uint8_t>(size >> 8 & 0xFF)); + buffer.push_back(static_cast<uint8_t>((size >> 8) & 0xFF));Apply similarly at line 394.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp(17 hunks)test/include/AsyncFunctionExecutorTests.hpp(2 hunks)test/include/MockClient.hpp(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/include/MockClient.hpp
🧰 Additional context used
🧠 Learnings (4)
📚 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/include/AsyncFunctionExecutorTests.hppinclude/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 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:
test/include/AsyncFunctionExecutorTests.hppinclude/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 Learning: 2025-11-03T10:39:48.715Z
Learnt from: MarioIvancik
Repo: bringauto/async-function-execution PR: 3
File: include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp:214-244
Timestamp: 2025-11-03T10:39:48.715Z
Learning: In the bringauto/async-function-execution repository, exceptions thrown from AsyncFunctionExecutor::callFunc() (specifically from client_->sendMessage(), client_->waitForMessage(), or deserializeReturn()) are considered fatal errors indicating unrecoverable infrastructure failures. The program is expected to terminate in such cases, so exception safety cleanup (like resetting callInProgress_ flags) is intentionally not implemented.
Applied to files:
test/include/AsyncFunctionExecutorTests.hppinclude/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
📚 Learning: 2024-10-22T11:03:16.796Z
Learnt from: MarioIvancik
Repo: bringauto/fleet-protocol-http-client-cxx PR: 13
File: example/main.cpp:30-32
Timestamp: 2024-10-22T11:03:16.796Z
Learning: In `example/main.cpp`, the code serves as an example implementation and may not require detailed error messaging enhancements.
Applied to files:
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp
🧬 Code graph analysis (1)
include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (3)
test/include/MockClient.hpp (10)
funcId(153-160)funcId(153-153)funcId(163-174)funcId(163-163)funcId(177-181)funcId(177-177)bytes(120-134)bytes(120-120)bytes(137-150)bytes(137-137)include/bringauto/async_function_execution/structures/Settings.hpp (4)
functionId(26-31)functionId(26-26)functionId(33-35)functionId(33-33)include/bringauto/async_function_execution/clients/AeronClient.hpp (1)
buffer(186-191)
🔇 Additional comments (13)
test/include/AsyncFunctionExecutorTests.hpp (4)
15-15: Good practice: explicit constructor prevents implicit conversions.Making the single-argument constructor explicit is correct and prevents unintended implicit conversions from
std::string.
55-59: LGTM: FunctionWait definition.The new
FunctionWaitdefinition with void return type and no arguments is correctly structured for testing timeout behavior.
74-81: Producer FunctionList correctly includes all configured functions.The updated FunctionList includes both
FunctionNoArgsandFunctionWait, consistent with the per-function configurations defined above.
85-105: Test setup correctly configured for asymmetric producer/consumer testing.The consumer configuration excludes
FunctionWaitwhile the producer includes it, allowing tests to exercise cross-executor behavior. The channel offset of 42 intentionally tests offset handling.include/bringauto/async_function_execution/AsyncFunctionExecutor.hpp (9)
10-10: LGTM: Headers and type updates support std::expected API.The
<expected>include and migration tostructures::FunctionConfigscorrectly support the new error-handling design.Also applies to: 23-23
29-29: Well-documented public API structures.The added documentation and explicit constructors clarify the API surface. Making FunctionDefinition members public enables cleaner client code.
Also applies to: 47-87
93-120: Robust compile-time constraints on FunctionList.The
IsFunctionDefinitionconcept and explicit constructor ensure type safety at compile time, preventing misuse of the FunctionList API.
124-133: Clear error taxonomy with CallError enum.The enumeration provides well-documented, type-safe error codes for all failure modes in function call flow.
214-244: Robust error handling with std::expected and per-function timeouts.The updated
callFunc()provides clear error semantics viastd::expectedand correctly applies per-function timeout configurations with a fallback todefaultTimeout. ThecallInProgress_flag prevents concurrent calls to the same function.
253-319: Improved safety in argument deserialization.The bounds checking at lines 300 and 303 prevents buffer overruns, and the clear error messages aid debugging. The deserialization logic correctly mirrors the encoding in
appendArg().
331-344: LGTM: sendReturnMessage() error signaling.Returning
intallows callers to detect send failures. The channel offset calculation correctly usesfunctionId.value + channelOffset_ + MESSAGE_RETURN_CHANNEL_OFFSET.
401-440: Solid implementation of std::expected-based deserialization and state tracking.The
deserializeReturn()correctly handles void specialization, serializable types, and trivially-copyable types. ThecallInProgress_map usingstd::atomic_boolensures thread-safe tracking of concurrent function calls.
181-201: No issues found - MESSAGE_RETURN_CHANNEL_OFFSET is properly defined and accessible.The constant
MESSAGE_RETURN_CHANNEL_OFFSETis defined inConstants.hpp(line 12) asconstexpr unsigned int MESSAGE_RETURN_CHANNEL_OFFSET = 1000;and is included transitively inAsyncFunctionExecutor.hppthrough theAeronClient.hppheader. All usages in theconnect()method are correct and consistent.
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Chores
Tests