Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#508] event concept based on socketpair #598

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Jan 20, 2025

Notes for Reviewer

Implemented the process local variant of events using socketpairs. Required - otherwise we do not have a real process local event variant, currently we use unix domain sockets with a file representation.

Windows required also a dup implementation on the platform since the notifier socket is acquired by duplicating one of the sockets from the socket pair since an event concept on that level is a M:1 connection (many notifiers and one listener) but a socketpair creates just 2 sockets.

Pre-Review Checklist for the PR Author

  • Add sensible notes for the reviewer
  • PR title is short, expressive and meaningful
  • Consider switching the PR to a draft (Convert to draft)
    • as draft PR, the CI will be skipped for pushes
  • Relevant issues are linked in the References section
  • Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  • Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  • Commits messages are according to this guideline
  • Tests follow the best practice for testing
  • Changelog updated in the unreleased section including API breaking changes
  • Assign PR to reviewer
  • All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  • All open points are addressed and tracked via issues

References

Relates to #508

@elfenpiff elfenpiff requested a review from elBoberido January 20, 2025 19:10
@elfenpiff elfenpiff self-assigned this Jan 20, 2025
@elfenpiff elfenpiff force-pushed the iox2-508-event-concept-based-on-socketpair branch from 94535f3 to 1b30800 Compare January 20, 2025 19:14
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 79.47977% with 71 lines in your changes missing coverage. Please review.

Project coverage is 78.86%. Comparing base (dd42fc9) to head (1b30800).

Files with missing lines Patch % Lines
iceoryx2-cal/src/event/process_local_socketpair.rs 82.93% 50 Missing ⚠️
iceoryx2-bb/posix/src/socket_pair.rs 65.00% 14 Missing ⚠️
iceoryx2-cal/src/dynamic_storage/process_local.rs 60.00% 4 Missing ⚠️
iceoryx2/src/port/notifier.rs 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   78.77%   78.86%   +0.08%     
==========================================
  Files         199      200       +1     
  Lines       24118    24443     +325     
==========================================
+ Hits        18999    19276     +277     
- Misses       5119     5167      +48     
Files with missing lines Coverage Δ
iceoryx2-cal/src/event/mod.rs 44.44% <ø> (ø)
iceoryx2/src/port/notifier.rs 88.14% <0.00%> (-1.06%) ⬇️
iceoryx2-cal/src/dynamic_storage/process_local.rs 93.06% <60.00%> (-1.01%) ⬇️
iceoryx2-bb/posix/src/socket_pair.rs 67.13% <65.00%> (-0.45%) ⬇️
iceoryx2-cal/src/event/process_local_socketpair.rs 82.93% <82.93%> (ø)

... and 10 files with indirect coverage changes

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. No obvious reason why the Windows CI fails.

I guess you need to add iceoryx2_bb_log to the iceoryx2-cal-tests bazel target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants