Skip to content

CON-382: Simplify concurrency guarantees#11

Merged
alejnp merged 22 commits intorticommunity:masterfrom
alejnp:feature/CON-382-thread-safety
Mar 5, 2026
Merged

CON-382: Simplify concurrency guarantees#11
alejnp merged 22 commits intorticommunity:masterfrom
alejnp:feature/CON-382-thread-safety

Conversation

@alejnp
Copy link
Member

@alejnp alejnp commented Feb 18, 2026

See #12.

@alejnp alejnp changed the title CON_382: Simplify concurrency guarantees CON-382: Simplify concurrency guarantees Feb 18, 2026
@alejnp alejnp force-pushed the feature/CON-382-thread-safety branch 2 times, most recently from a6cc82e to e2459f8 Compare February 19, 2026 13:07
@alejnp alejnp force-pushed the feature/CON-382-thread-safety branch from e2459f8 to 5e80bca Compare February 19, 2026 13:09
@alejnp alejnp marked this pull request as ready for review February 19, 2026 14:22
Copy link
Contributor

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 simplifies the concurrency model for the RTI Connector Rust bindings by removing the complex Acquire/Release pattern and replacing it with a simpler Mutex-based approach. The change improves user experience at the cost of minor performance overhead on synchronization operations.

Changes:

  • Replaced RwLock-based acquire/release pattern with Arc for thread-safe connector access
  • Removed lifetimes from Input and Output types, making them cloneable and easier to use
  • Deprecated take_input/take_output methods in favor of simplified get_input/get_output
  • Updated documentation to reflect the simplified threading model

Reviewed changes

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

Show a summary per file
File Description
src/connector.rs Replaced complex ThreadSafeEntityHolder with Arc containing Mutex; removed acquire/release logic; deprecated take_* methods
src/input.rs Removed lifetime parameter; changed to Arc-based ownership; removed Drop implementation; simplified native access
src/output.rs Removed lifetime parameter; changed to Arc-based ownership; removed Drop implementation; simplified native access
src/ffi/mod.rs Added Send implementation for FfiConnector to enable Arc<Mutex>
src/lib.rs Added concurrency trait tests to verify Send + Sync implementations
tests/test_input.rs Updated tests to expect concurrent get_input to succeed; changed take_input to get_input
tests/test_output.rs Updated tests to expect concurrent get_output to succeed; changed take_output to get_output
tests/test_utils/context.rs Removed lifetime from TestEntities struct
docs/guide/threading.md Simplified threading documentation to reflect new model
docs/guide/connector.md Updated note about multi-threaded access
docs/guide/input.md Removed lifetime from type signatures
docs/guide/output.md Removed lifetime from type signatures
docs/lib.md Changed heading level from # to ##
examples/shapes/publisher.rs Changed take_output to get_output
examples/shapes/subscriber.rs Changed take_input to get_input
snippets/quickstart.rs Removed lifetimes from type annotations
snippets/output/using_instance.rs Removed lifetime from function signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

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

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

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

Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (2)

examples/shapes/subscriber.rs:41

  • The error context still says "Failed to take input" even though the code now calls get_input. Updating this message will make failures easier to interpret when debugging.
    let mut input = connector
        .get_input(INPUT_NAME)
        .map_err(|e| format!("Failed to take input: {}", e))?;

examples/shapes/publisher.rs:44

  • The error context still says "Failed to take output" even though the code now calls get_output. Consider updating the message so it matches the actual operation being performed.
    let mut output = connector
        .get_output(OUTPUT_NAME)
        .map_err(|e| format!("Failed to take output: {}", e))?;


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Reimplement Acquire/Release
Remove Clone from Input/Output
Remove unneeded threading guarantees
Copy link
Contributor

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

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/output.rs:155

  • The lifetime parameter 'a in the Drop implementation is unnecessary and unused since Output no longer has any lifetime parameters. Consider removing it for clarity.
impl<'a> Drop for Output {

src/input.rs:321

  • Grammar error: "This samples will be discard" should be "These samples will be discarded".
    /// This samples will be discard by the [`Input`] next time either

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alejnp alejnp requested a review from jeholliday March 2, 2026 14:52
@alejnp alejnp requested a review from alexcamposruiz March 4, 2026 10:50
@alejnp alejnp dismissed alexcamposruiz’s stale review March 5, 2026 11:31

We've discussed this offline and we have agreed that the current approach is robust enough.

@alejnp alejnp removed the request for review from alexcamposruiz March 5, 2026 11:31
@alejnp alejnp merged commit e678227 into rticommunity:master Mar 5, 2026
4 checks passed
@alejnp alejnp deleted the feature/CON-382-thread-safety branch March 5, 2026 11:32
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.

4 participants