Rust::com FFI mock implementation for unit test by bharatGoswami8 · Pull Request #388 · eclipse-score/communication

bharatGoswami8 · 2026-05-06T05:06:02Z

Added a native FFI bridge and a mock FFI bridge for deterministic local testing.
Runtime, consumer, and producer APIs are now pluggable via a bridge abstraction to support interchangeable backends.
Added an explicit error for requests that exceed the maximum allowed sample count.
Added mock-backed tests

rpreddyhv

Looks good to me.

darkwisebear

I think the idea is fine. However, the approach with the thread locals bothers me: Why not just instantiate the bridge type and hold test data inside the bridge type if necessary? The extra self parameter needed for the methods are ok imo.

darkwisebear · 2026-05-13T12:04:09Z

+    /// # Safety
+    /// `callback` must be a valid `FatPtr` referencing a callable compatible with the
+    /// find-service callback signature. `instance_spec` must be a valid `InstanceSpecifier`.
+    /// The returned handle must eventually be passed to `stop_find_service`.
+    unsafe fn start_find_service(


I think this unsafety is misplaced. Afaiu, the main point is that FatPtr is essentially a type-erased Rust fat ptr. However, we could turn this method into something safe if we wrap the FatPtr into a FindServiceCallable and embed the FatPtr inside this type. The invariant of this type would then be the guarantee that it points to the appropriate callback. If the Rust compiler cannot verify that, then the unsafety is pushed to the point where this new type is instantiated. And this is imo the better place, since it is directly at the point where the original type gets "encoded into the Rust type system".

I have created an issue/ task for this, will investigate and optimize the unsafe call in rust FFI.
#431

darkwisebear · 2026-05-13T12:15:09Z

+    /// `container` must be a valid reference to a `HandleContainer` that was produced by the
+    /// bridge (e.g. from `start_find_service` callback). Calling this on a sentinel mock pointer
+    /// is only safe when the mock implementation ignores the inner pointer.
+    unsafe fn handle_container_size(container: &HandleContainer) -> usize;


Same. If it is possible to safely construct a HandleContainer, the method's safety should not rely on the content being correct. If there is a precondition for the HandleContainer to be correct, this shall be pushed to construction time.

I just asked myself why these methods are there in the first place... Why not let HandleContainer do that job?

HandleContainer is still part of the proxy_bridge(old API implementation) and we don't have generic or mock support, anyways we are going to clean-up the old API implementation and required part will move in bridge folder.
(#432)
For bridge as we have enabled the mock support so rather than calling direct from proxy_bridge, we are using from this file.

Unsafe removed from method signature.

darkwisebear · 2026-05-13T12:42:11Z

+    /// # Safety
+    /// Same as `handle_container_size`. `index` need not be in-bounds; the method must return
+    /// `None` when `index >= handle_container_size(container)`.
+    unsafe fn handle_container_get_at(


unsafe also unclear.

Removed as not required.

darkwisebear · 2026-05-13T12:55:10Z

Please enable Rust 2024 edition for all targets.

Added ffi BUILD as well as another COM-API lib related target also.

darkwisebear · 2026-05-13T19:21:34Z

+// As of now, it just provide basic mock implementation and returning values based on
+// input parameters. In future, we will enhance this mock implementation to verify
+// all the test cases and scenarios.
+#![doc(hidden)]


Why hide the mock backend?

It was included because, as it is only needed for internal unit testing, so I didn’t see the need to expose it in the crate-level documentation.

darkwisebear · 2026-05-13T19:22:49Z

+// input parameters. In future, we will enhance this mock implementation to verify
+// all the test cases and scenarios.
+#![doc(hidden)]
+#![allow(clippy::missing_safety_doc)]


Yes, it's test code, but I don't see how this justifies skipping the safety comments. I'd rather say we can afford things that allocate more and are less performant in exchange to using less unsafe code instead.

Removed the clippy lint and added safety comment on required places.

darkwisebear · 2026-05-13T19:23:17Z

+use std::cell::RefCell;
+
+// Type alias for the heap-pointer + drop-glue pair stored in backing thread-locals.
+type BackingEntry = (*mut std::ffi::c_void, fn(*mut std::ffi::c_void));


I prefer a dedicated struct here. This way, you can name the parts and it's clear what part does what.

changed to struct type

darkwisebear · 2026-05-13T19:24:50Z

+type BackingEntry = (*mut std::ffi::c_void, fn(*mut std::ffi::c_void));
+
+// The thread-local storage is used to hold the backing data for the mock FFI bridge.
+thread_local! {


Why thread_local!? Because each test is run in a separate thread..? I think it would make more sense to generalize all the instance pointers in the trait and use them to store mock data. Or you just add self parameters and use the usual mocking methods instead of relying on static data. Imo that's almost always a smell, also in test code.

yes it was because of test running in multithread environment but as you suggested i have optimized the MockFFI and Runtime implementation to use the instance of bridge and &self in ffi method signature.

darkwisebear · 2026-05-13T19:29:33Z


    // Initialise the Lola runtime.
-    let mut runtime_builder = LolaRuntimeBuilderImpl::new();
+    let mut runtime_builder: LolaRuntimeBuilderImpl = LolaRuntimeBuilderImpl::new();


Isn't that redundant? Why is this necessary?

Compiler is not able to resolve the FFIBridge even though there is default and complaining about it

note: cannot satisfy `_: bridge_ffi_rs::FFIBridge` help: the trait `bridge_ffi_rs::FFIBridge` is implemented for `bridge_ffi_lola::LolaFFIBridge`

explicit type annotation gives the compiler enough information to resolve the generic parameter.

darkwisebear · 2026-05-13T19:31:13Z

-    type ProviderInfo = LolaProviderInfo;
-    type ConsumerInfo = LolaConsumerInfo;
+pub struct LolaRuntimeImpl<B: FFIBridge = LolaFFIBridge> {
+    _marker: PhantomData<B>,


Why not just B?

we do not need a field with type with B , if we keep B then we need to construct instance and we don't need FFIBridge instance in LolaRuntimeImpl or runtime trait impl.

But only because you use static data to keep the mock data. If you had an instance here, you might keep the data there. Much cleaner imo.

understood, optimized the MockFFI and Runtime implementation to use the instance of bridge and &self in method signature

* FFI mock api implemented for runtime unit test

* FFIBridge as generic parameter so that can be change between mock and Lola FFI

* Added unit test using FFI mock

* Moved Lola specific FFI implementation in seperate file

* Updated HandleContainer method for Lola and Mock

* Added global variable cleanup * Updated validation check on test

* remove unsafe from handleContainer support function from FFI * Updated FFI type alias to struct type

* Updated rust edition to 2024 in bazel com-api related rust target

* Taking FFI bridge as intance on runtime impl * Removed static init in mock ffi * Added paramter in mock type * Calling to FFI function in runtime updated

bharatGoswami8 · 2026-05-15T09:51:04Z

I think the idea is fine. However, the approach with the thread locals bothers me: Why not just instantiate the bridge type and hold test data inside the bridge type if necessary? The extra self parameter needed for the methods are ok imo.

@darkwisebear, I have updated the mock backend, Instead of using thread_local!, I have adopted the approach you suggested - instantiating the bridge type and storing the test data within it.

* Added centralized ARC in producer and consumer info

bharatGoswami8 requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners May 6, 2026 05:06

bharatGoswami8 requested a review from darkwisebear May 6, 2026 05:06

bharatGoswami8 self-assigned this May 6, 2026

bharatGoswami8 added the rust-api label May 6, 2026

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from 3ddde38 to c33d943 Compare May 6, 2026 05:25

tejaswinijoshi89 approved these changes May 6, 2026

View reviewed changes

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from 85c633c to 94efa87 Compare May 6, 2026 06:44

rpreddyhv approved these changes May 11, 2026

View reviewed changes

darkwisebear suggested changes May 13, 2026

View reviewed changes

bharatGoswami8 added 11 commits May 14, 2026 11:43

Rust::com FFI mock implementation for unit test

171f5b7

* FFI mock api implemented for runtime unit test

Rust::com Interface trait for ffi methods

52fc2eb

Rust::com Added FFI Bridge generic parameter

d07d70f

* FFIBridge as generic parameter so that can be change between mock and Lola FFI

Rust::com Unit test for consumer and producer

590c0b4

* Added unit test using FFI mock

Rust::com move Lola FFI implementation

8378fd1

* Moved Lola specific FFI implementation in seperate file

Rust::com Mock implementaion updated for allocatee apis

a886841

Rust::com HandleContainer get method update

ac3844b

* Updated HandleContainer method for Lola and Mock

Rust::com updated SamplePtr mock implementation

82f81c2

Rust::com FFI mock and unit test for runtime

978aca0

* Added global variable cleanup * Updated validation check on test

Rust::com Integration test fix

e8b5d93

Rust::com Optimized the Mock FFI

208754c

* remove unsafe from handleContainer support function from FFI * Updated FFI type alias to struct type

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch 3 times, most recently from 79c4959 to d15ac90 Compare May 14, 2026 11:23

Rust::com Update Rust 2024 edition for all the com-api related target

2495ad2

* Updated rust edition to 2024 in bazel com-api related rust target

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from d15ac90 to 2495ad2 Compare May 14, 2026 11:36

bharatGoswami8 mentioned this pull request May 14, 2026

Improvement: Optimization of unsafe call in rust FFI #431

Open

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch 2 times, most recently from 53c4061 to e014b35 Compare May 15, 2026 08:38

Rust::com FFI bridge as instance on Runtime

e5aee85

* Taking FFI bridge as intance on runtime impl * Removed static init in mock ffi * Added paramter in mock type * Calling to FFI function in runtime updated

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from e014b35 to e5aee85 Compare May 15, 2026 09:23

bharatGoswami8 requested a review from darkwisebear May 15, 2026 09:46

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch 2 times, most recently from c73650c to ae47262 Compare May 15, 2026 11:16

Rust::com Optimmized the bridge clone in runtime

5be1bf8

* Added centralized ARC in producer and consumer info

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from ae47262 to 5be1bf8 Compare May 15, 2026 12:00

Conversation

bharatGoswami8 commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpreddyhv left a comment

Choose a reason for hiding this comment

Uh oh!

darkwisebear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatGoswami8 May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatGoswami8 May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatGoswami8 commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bharatGoswami8 commented May 6, 2026 •

edited

Loading

bharatGoswami8 May 14, 2026 •

edited

Loading

bharatGoswami8 May 14, 2026 •

edited

Loading