eclipse-iceoryx / iceoryx2

Eclipse iceoryx2™ - true zero-copy inter-process-communication in pure Rust
https://iceoryx.io
Apache License 2.0
1.03k stars 40 forks source link

[#349] Add service name generator to be able to run tests concurrently #472

Closed elfenpiff closed 1 month ago

elfenpiff commented 1 month ago

Notes for Reviewer

Fixes some parts of the problem but not everything

Pre-Review Checklist for the PR Author

  1. [x] Add sensible notes for the reviewer
  2. [x] PR title is short, expressive and meaningful
  3. [x] Relevant issues are linked in the References section
  4. [x] Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. [x] Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. [x] Commits messages are according to this guideline
    • [x] Commit messages have the issue ID ([#123] Add posix ipc example)
    • [x] Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  7. [x] Tests follow the best practice for testing
  8. [x] Changelog updated in the unreleased section including API breaking changes
  9. [x] Assign PR to reviewer
  10. [x] All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

  1. [ ] All open points are addressed and tracked via issues

References

Relates to #349 Closes #475

elfenpiff commented 1 month ago

When set_log_level(LogLevel::TRACE) is active one test fails with this:

ServiceEventTest/0.open_or_create_service_does_exist

      353 [D] Builder { storage_name: FileName { value: FixedSizeByteString<255> { len: 40, data: "26380db3f2d1d21a17def8273faec2e5097cf3e2" } }, supplementary_size: 0, ha
s_ownership: false, config: Configuration { suffix: FileName { value: FixedSizeByteString<255> { len: 8, data: ".dynamic" } }, prefix: FileName { value: FixedSizeByteStrin
g<255> { len: 5, data: "iox2_" } }, path: Path { value: FixedSizeByteString<255> { len: 14, data: "/tmp/iceoryx2/" } }, _data: PhantomData<iceoryx2::service::dynamic_confi
g::DynamicConfig> }, timeout: 500ms, initializer: , _phantom_data: PhantomData<iceoryx2::service::dynamic_config::DynamicConfig> }
| Failed to open  since the version number was not set - (it is not initialized after 500ms).

Which then lead to a failure when opening the dynamic service info and then the builder assumes the services is in a corrupted state. This comes only up with bazel, never occurs with cmake or with Rust (which has the same test)

The mechanism that seems to fail here is the concurrent dynamic storage creation/opening for the ipc::Service. To support concurrent creation/opening the creation must somehow tell the opener that the initialization is still in progress. This is done with two mechanisms:

  1. The permission is set to write only as long as the memory is initializing.
  2. The first 8 byte (or so) are zeroed by the OS and
  3. when the initialization is finished the version number is written.
  4. The permission is set to read/write

When the opener comes it first checks if it has read/write permissions, if not, initialization is still in progress. If it has read/write permissions it also checks the version number. If it is zero, still in initialization, if not zero it must match its own local version number.

elfenpiff commented 1 month ago

Okay, there was an actual bug in the dynamic storage posix shared memory implementation. When the creation process is still initializing the memory, the opener takes a COPY of the version number instead of a pointer to it and checks it until it becomes non-zero.

The thing is, this is a bug that should only occur on OSes that do not support adjusting the permissions of the shm during runtime like Mac OS and Windows and not linux.

And this also does not fix the failing test

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.92%. Comparing base (cd27eee) to head (b3dfa90). Report is 27 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472/graphs/tree.svg?width=650&height=150&src=pr&token=FN3YFXTJCI&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) ```diff @@ Coverage Diff @@ ## main #472 +/- ## ========================================== - Coverage 78.93% 78.92% -0.01% ========================================== Files 197 198 +1 Lines 23637 23627 -10 ========================================== - Hits 18657 18647 -10 Misses 4980 4980 ``` | [Files with missing lines](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | Coverage Δ | | |---|---|---| | [iceoryx2-bb/elementary/src/package\_version.rs](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472?src=pr&el=tree&filepath=iceoryx2-bb%2Felementary%2Fsrc%2Fpackage_version.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eDItYmIvZWxlbWVudGFyeS9zcmMvcGFja2FnZV92ZXJzaW9uLnJz) | `87.50% <100.00%> (+17.50%)` | :arrow_up: | | [...yx2-cal/src/dynamic\_storage/posix\_shared\_memory.rs](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472?src=pr&el=tree&filepath=iceoryx2-cal%2Fsrc%2Fdynamic_storage%2Fposix_shared_memory.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#diff-aWNlb3J5eDItY2FsL3NyYy9keW5hbWljX3N0b3JhZ2UvcG9zaXhfc2hhcmVkX21lbW9yeS5ycw==) | `89.29% <100.00%> (-0.25%)` | :arrow_down: | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/472/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)