Skip to content

[#429] add response channel #680

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

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

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Apr 7, 2025

Notes for Reviewer

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 #429

@elfenpiff elfenpiff force-pushed the iox2-429-add-response-channel branch 5 times, most recently from 4ede017 to 4b0f3b9 Compare April 15, 2025 12:34
@elfenpiff elfenpiff force-pushed the iox2-429-add-response-channel branch from baef1a0 to b1f003e Compare April 17, 2025 18:11
@elfenpiff elfenpiff changed the title [DO NOT REVIEW] Iox2 429 add response channel [#429] add response channel Apr 18, 2025
@elfenpiff elfenpiff marked this pull request as ready for review April 18, 2025 20:01
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new response channel mechanism and augments the request/response API to better support both zero-copy and copy-based responses. Key changes include:

  • Adding new configuration fields in the config module to control client buffer size and fire-and-forget request behavior.
  • Extending the ActiveRequest type with new fields (e.g. shared_state, request_id, channel_id, connection_id) and methods (e.g. loan_uninit, send_copy, loan) to manage response channels.
  • Removing obsolete FFI API functions and updating related tests and zero-copy connection components to support enhanced channel state management.

Reviewed Changes

Copilot reviewed 53 out of 54 changed files in this pull request and generated no comments.

Show a summary per file
File Description
iceoryx2/src/config.rs Added new configuration fields supporting response channel behavior.
iceoryx2/src/active_request.rs Extended ActiveRequest with new fields and response loan/send APIs.
iceoryx2-ffi/ffi/src/api/subscriber.rs & subscriber.hpp Removed the update_connections API from the FFI layer.
iceoryx2-cal/* Updated tests and internal components to support channel state handling.
iceoryx2-bb/* & iceoryx2-cal/* Adjustments in dynamic storage and container implementations.
examples/rust/request_response/* Updated examples to demonstrate usage of both response APIs.
Files not reviewed (1)
  • iceoryx2-cal/BUILD.bazel: Language not supported
Comments suppressed due to low confidence (4)

iceoryx2/src/config.rs:358

  • [nitpick] The flag 'allow_fire_and_forget_requests' might be ambiguous; consider renaming it to 'enable_fire_and_forget_requests' to clearly indicate that this flag enables the behavior.
pub allow_fire_and_forget_requests: bool,

iceoryx2-ffi/ffi/src/api/subscriber.rs:390

  • [nitpick] The removal of the update_connections function from the FFI API should be accompanied by updated documentation and tests to ensure that consumers of the API are adequately informed of this change.
/* removed update_connections function */

iceoryx2/src/active_request.rs:158

  • Replacing fatal_panic! with error! changes critical error handling semantics. Verify that logging the error without panicking is the intended behavior when the client's retrieval channel is full.
error!(from self, "This should never happen! The clients retrieve channel is full and the request cannot be returned.");

iceoryx2-bb/container/src/vec.rs:302

  • Ensure that the memory move in remove_impl, which uses core::ptr::copy, correctly handles overlapping memory regions. Although core::ptr::copy allows for overlap, verify that the logic is safe in all scenarios.
unsafe fn remove_impl(&mut self, index: usize) -> T {

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.

1 participant