Catch reentrant calls to FSM and HFSM start, receive, etc. #1202
Conversation
|
@jwellbelove, sorry for spamming your inbox this month. This is the last item I should need to push for a while. |
WalkthroughAdds reentrancy protection to the FSM/HFSM code paths. A new public exception Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/etl/fsm.h(9 hunks)include/etl/hfsm.h(5 hunks)test/CMakeLists.txt(1 hunks)test/test_hfsm_transition_on_enter.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
include/etl/*.h
⚙️ CodeRabbit configuration file
include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.Header files should start with the ETL's standard MIT comment block.
Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'
Headers should be organized in the following order
- The first header include should be '#include "platform.h"'
- Headers from the ETL
- Headers from the path 'private/**'
- C++ standard library headers
- C standard library headers
Pay extra attention to the following
- Buffer overflows
- Memory leaks
- Undefined behavior
Check that the following conventions are used
- Variable names should be in
snake_case- Macro constants should be
ALL_CAPS_WITH_UNDERSCORES- Macro constants should be prefixed with 'ETL_'
- Function, class, and type names should be
snake_case- enum members should be
Caps_Snake_CasePlease keep your outputs short and to the point, unless otherwise asked
- Keep your responses short, describe small issues in a few sentences
- Don't output tips, analysis chains or anything else with a collapsible view
- Don't output
comment summariesorfix prompts for AIs- If the highlighted issue is complex, don't suggest a solution
- Don't use emojis
Files:
include/etl/hfsm.hinclude/etl/fsm.h
🧬 Code graph analysis (3)
include/etl/hfsm.h (1)
include/etl/fsm.h (9)
fsm_reentrancy_guard(231-257)etl(71-640)etl(679-708)fsm_null_state_exception(114-122)ifsm_state(324-386)ifsm_state(405-407)fsm_state_id_exception(127-135)fsm_state_id_t(419-419)is_self_transition(671-674)
test/test_hfsm_transition_on_enter.cpp (1)
include/etl/fsm.h (1)
fsm_reentrant_transition_forbidden(179-186)
include/etl/fsm.h (2)
include/etl/generators/fsm_generator.h (4)
fsm_not_started(197-204)etl(663-691)fsm_exception(120-968)is_self_transition(655-658)include/etl/hfsm.h (1)
etl(34-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clang C++23 OSX - No STL (macos-13)
- GitHub Check: Clang C++20 OSX - No STL (macos-13)
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL (macos-13)
- GitHub Check: Clang C++20 OSX - STL - Force C++03 (macos-13)
- GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
- GitHub Check: Windows - No STL - Force C++03
- GitHub Check: Windows - STL - Force C++03
- GitHub Check: Windows - STL
- GitHub Check: Windows - No STL
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
… states in FSM/HFSM
ockeydockey
force-pushed
the
feature/fsm_detect_reentrant_calls
branch
from
180dd10 to
9114679
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/etl/fsm.h (1)
226-257: LGTM. Consider explicitly deleting copy operations.The RAII guard is correctly implemented. The constructor properly checks the flag before setting it, and the destructor is marked
ETL_NOEXCEPT.For defensive coding, consider explicitly deleting copy and move operations:
private: // Reference to the flag signifying a lock on the state machine. bool& is_locked; + + // Disable copy and move + fsm_reentrancy_guard(const fsm_reentrancy_guard&) ETL_DELETE; + fsm_reentrancy_guard& operator=(const fsm_reentrancy_guard&) ETL_DELETE; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/etl/fsm.h(8 hunks)include/etl/hfsm.h(2 hunks)test/test_hfsm_transition_on_enter.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/etl/hfsm.h
- test/test_hfsm_transition_on_enter.cpp
🧰 Additional context used
📓 Path-based instructions (1)
include/etl/*.h
⚙️ CodeRabbit configuration file
include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.Header files should start with the ETL's standard MIT comment block.
Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'
Headers should be organized in the following order
- The first header include should be '#include "platform.h"'
- Headers from the ETL
- Headers from the path 'private/**'
- C++ standard library headers
- C standard library headers
Pay extra attention to the following
- Buffer overflows
- Memory leaks
- Undefined behavior
Check that the following conventions are used
- Variable names should be in
snake_case- Macro constants should be
ALL_CAPS_WITH_UNDERSCORES- Macro constants should be prefixed with 'ETL_'
- Function, class, and type names should be
snake_case- enum members should be
Caps_Snake_CasePlease keep your outputs short and to the point, unless otherwise asked
- Keep your responses short, describe small issues in a few sentences
- Don't output tips, analysis chains or anything else with a collapsible view
- Don't output
comment summariesorfix prompts for AIs- If the highlighted issue is complex, don't suggest a solution
- Don't use emojis
Files:
include/etl/fsm.h
🧬 Code graph analysis (1)
include/etl/fsm.h (2)
include/etl/hfsm.h (1)
etl(34-111)include/etl/generators/fsm_generator.h (2)
etl(663-691)fsm_exception(120-968)
🔇 Additional comments (3)
include/etl/fsm.h (3)
466-466: LGTM.The member variable is correctly initialised to false in the constructor initialiser list.
517-518: LGTM.The guard correctly protects the
start()method from reentrant calls.
549-550: LGTM.The guard correctly protects
receive(),transition_to(), andreset()from reentrant calls. All state-changing public methods are now properly guarded.Also applies to: 568-569, 632-633
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
include/etl/fsm.h (1)
175-186: Typo in error message already flagged.The double space in "to start" on line 183 has already been identified in a previous review.
🧹 Nitpick comments (1)
include/etl/fsm.h (1)
226-257: Consider deleting copy and move operations for the RAII guard.The
fsm_reentrancy_guardimplementation is correct, but as a best practice for RAII types, consider explicitly deleting the copy constructor, copy assignment operator, move constructor, and move assignment operator. This prevents accidental copying or moving of the guard, which would result in the flag being reset multiple times.Since this file is auto-generated from
fsm_generator.h, this change should be applied to the generator template.Apply this diff in the generator template:
private: // Reference to the flag signifying a lock on the state machine. bool& is_locked; + + // Deleted copy and move operations + fsm_reentrancy_guard(const fsm_reentrancy_guard&) ETL_DELETE; + fsm_reentrancy_guard& operator=(const fsm_reentrancy_guard&) ETL_DELETE; +#if ETL_USING_CPP11 + fsm_reentrancy_guard(fsm_reentrancy_guard&&) ETL_DELETE; + fsm_reentrancy_guard& operator=(fsm_reentrancy_guard&&) ETL_DELETE; +#endif };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/etl/fsm.h(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
include/etl/*.h
⚙️ CodeRabbit configuration file
include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.Header files should start with the ETL's standard MIT comment block.
Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'
Headers should be organized in the following order
- The first header include should be '#include "platform.h"'
- Headers from the ETL
- Headers from the path 'private/**'
- C++ standard library headers
- C standard library headers
Pay extra attention to the following
- Buffer overflows
- Memory leaks
- Undefined behavior
Check that the following conventions are used
- Variable names should be in
snake_case- Macro constants should be
ALL_CAPS_WITH_UNDERSCORES- Macro constants should be prefixed with 'ETL_'
- Function, class, and type names should be
snake_case- enum members should be
Caps_Snake_CasePlease keep your outputs short and to the point, unless otherwise asked
- Keep your responses short, describe small issues in a few sentences
- Don't output tips, analysis chains or anything else with a collapsible view
- Don't output
comment summariesorfix prompts for AIs- If the highlighted issue is complex, don't suggest a solution
- Don't use emojis
Files:
include/etl/fsm.h
🧬 Code graph analysis (1)
include/etl/fsm.h (2)
include/etl/hfsm.h (1)
etl(34-111)include/etl/generators/fsm_generator.h (2)
etl(663-691)fsm_exception(120-968)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clang C++20 OSX - No STL (macos-13)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL (macos-13)
- GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL - Force C++03 (macos-13)
- GitHub Check: Clang C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - No STL (macos-13)
- GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - No STL - Force C++03 (macos-13)
- GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
🔇 Additional comments (6)
include/etl/fsm.h (6)
466-466: LGTM!The initialisation of
is_processing_state_changetofalsein the constructor is correct.
517-518: LGTM!The reentrancy guard is correctly placed at the entry of
start()to prevent reentrant calls during FSM startup.
549-550: LGTM!The reentrancy guard correctly protects the
receive()method from reentrant calls during message processing and state transitions.
568-569: LGTM!The reentrancy guard is properly placed in
transition_to()to prevent reentrant transition requests.
632-633: LGTM!The reentrancy guard correctly protects the
reset()method from reentrant calls during FSM reset operations.
713-713: LGTM!The member variable declaration for
is_processing_state_changeis correct, follows naming conventions, and includes a clear descriptive comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/etl/fsm.h (1)
226-263: Reentrancy guard is sound; document single‑thread assumptionRAII, assert, and deleted copy/move are appropriate. Please ensure docs clearly state it is not thread‑safe (not a mutex) and requires external serialisation for multi‑threaded callers.
include/etl/generators/fsm_generator.h (1)
244-283: fsm_reentrancy_guard implementationRAII design, assert usage, and noexcept dtor are good. Keep the “not a mutex” comment; consider adding a brief note in the public docs of fsm/hfsm about single‑threaded reentrancy only.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/etl/fsm.h(8 hunks)include/etl/generators/fsm_generator.h(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
include/etl/*.h
⚙️ CodeRabbit configuration file
include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.Header files should start with the ETL's standard MIT comment block.
Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'
Headers should be organized in the following order
- The first header include should be '#include "platform.h"'
- Headers from the ETL
- Headers from the path 'private/**'
- C++ standard library headers
- C standard library headers
Pay extra attention to the following
- Buffer overflows
- Memory leaks
- Undefined behavior
Check that the following conventions are used
- Variable names should be in
snake_case- Macro constants should be
ALL_CAPS_WITH_UNDERSCORES- Macro constants should be prefixed with 'ETL_'
- Function, class, and type names should be
snake_case- enum members should be
Caps_Snake_CasePlease keep your outputs short and to the point, unless otherwise asked
- Keep your responses short, describe small issues in a few sentences
- Don't output tips, analysis chains or anything else with a collapsible view
- Don't output
comment summariesorfix prompts for AIs- If the highlighted issue is complex, don't suggest a solution
- Don't use emojis
Files:
include/etl/fsm.h
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/fsm_generator.h
🧬 Code graph analysis (2)
include/etl/fsm.h (2)
include/etl/generators/fsm_generator.h (5)
fsm_reentrant_transition_forbidden(198-205)etl(83-672)etl(711-740)fsm_exception(120-574)fsm_reentrancy_guard(250-282)include/etl/hfsm.h (1)
etl(34-111)
include/etl/generators/fsm_generator.h (1)
include/etl/fsm.h (7)
fsm_not_started(166-173)etl(71-646)etl(685-714)fsm_exception(101-548)fsm_reentrant_transition_forbidden(179-186)fsm_reentrancy_guard(231-263)is_self_transition(677-680)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - No STL (macos-13)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - STL
- GitHub Check: Windows - No STL
- GitHub Check: Windows - No STL - Force C++03
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Windows - STL - Force C++03
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
🔇 Additional comments (18)
include/etl/fsm.h (9)
175-186: New exception looks correctConstructor and error text align with ETL patterns. LGTM.
224-225: No functional changeWhitespace/formatting only.
472-472: Member initialiser addedInitialising is_processing_state_change to false in ctor is correct. LGTM.
523-524: Guarding start()Good placement; ensures reentrant start is caught early.
555-556: Guarding receive()Prevents nested receives during transitions. LGTM.
574-575: Guarding transition_to()Consistent with start/receive. LGTM.
638-639: Guarding reset()Correct to protect against reentrant reset paths.
713-718: Self‑transition handling – confirm intended semanticsThis calls on_exit_state(), re‑enters, and may call on_exit_state() again if a different next state is chosen during on_enter. If double exit is intended, fine; otherwise, consider adjusting.
719-720: New reentrancy state flagPrivate bool tracking active state changes is appropriate. LGTM.
include/etl/generators/fsm_generator.h (9)
183-190: fsm_not_started placement/textMatches runtime usage and ETL error text style. LGTM.
195-205: New fsm_reentrant_transition_forbiddenAPI and message are consistent with generated header and tests. LGTM.
498-499: Constructor initialiser updatedInitialising is_processing_state_change is correct. LGTM.
549-550: Guard in start()Correct and consistent with header. LGTM.
581-582: Guard in receive()Prevents nested/event‑triggered reentry. LGTM.
600-601: Guard in transition_to()Consistent application across entry points. LGTM.
664-665: Guard in reset()Correct placement. LGTM.
713-718: Self‑transition handling – verifySame semantics as generated header: potential double on_exit_state if on_enter triggers change. Please confirm this is intended.
745-746: New member: is_processing_state_changeAppropriate addition; remains private in generated class. LGTM.
ockeydockey
changed the title
ockeydockey
changed the title
1 participant
Adding the ability to catch when a reentrant call to a method which can change state has been made. Since transitioning states while another state transition is active could cause state corruption, infinite loops, or other issues, this new mechanism will fail an assert.
The most common example would be a state transition (due to calling
receive()) triggering another call toreceive()before the state transitions have finished processing. This second call will now throw an error.