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: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.

Ok, this is reasonable. Maybe you want to clarify the comment above the attribute then?

Update the comment for attribute.

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.

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.

darkwisebear

We're not there yet. Please consider using a real mock for the bridge that implements all the trait methods and allows for setting call reactions on a per-test basis with defined values that are different from dangling pointers and other stuff that scares me.

darkwisebear · 2026-05-22T13:03:56Z

+    data_backing: Arc<Mutex<Option<BackingEntry>>>,
+    //ALLOC_SIZE holds the size of the allocatee type for the next get_allocatee_ptr call, allowing
+    //the mock to zero-fill the caller's slot correctly.
+    alloc_size: Arc<Mutex<usize>>,
+    //SAMPLE_BACKING holds a pointer to the heap-allocated sample data and a drop function to free it.
+    sample_backing: Arc<Mutex<Option<BackingEntry>>>,


Please merge these three entries by creating a struct that contains the three members and put it behind an Arc<Mutex<MockFFIBridgeState>>.

Merged all the mock related types with an MockFFIBridgeState and using with one Arc instance under MockFFIBridge

darkwisebear · 2026-05-22T13:14:41Z

+    unsafe fn get_allocatee_ptr(
+        &self,
+        event_ptr: *mut SkeletonEventBase,
+        allocatee_ptr: *mut std::ffi::c_void,


Sorry to ask this so late, but isn't the type of this pointer something known? Why does this have to be c_void?

Type T known at rust side compile time but we were not using generic in FFI functions/methods in old static based function call, but with new trait and FFI instance-based approach, i will investigate with #431 with this ticket.

darkwisebear · 2026-05-22T13:14:57Z

+        if size == 0 {
+            return false;
+        }
+        unsafe { std::ptr::write_bytes(allocatee_ptr as *mut u8, 0, size) };


Why does this line exist?

I have updated allocation with setter function like mockall expect function.

darkwisebear · 2026-05-22T13:16:09Z

+            return false;
+        }
+        let size = *self.alloc_size.lock().expect("Failed to lock alloc_size");
+        if size == 0 {


If 0 has a special meaning, you should use an Option<NonZeroUsize> instead of using a magic number.

Updated with Option<NonZeroUsize>

darkwisebear · 2026-05-22T13:33:40Z

+    // `ProxyEventBase` is a ZST opaque type, a dangling non-null pointer is the
+    // canonical representation for "valid but empty" in the mock.


In which universe is a dangling pointer a "valid but empty representation"..? I really think it's a wording issue but it's just so disturbing. Please consider the mock approach suggested above.

Added unit type for Proxy/Skeleton/Event and constructing using setter methods, so each test can individually construct related types and use it as per test cases.

darkwisebear · 2026-05-22T13:34:18Z

+        _allocatee_ptr: *const std::ffi::c_void,
+        _type_name: &str,
+    ) -> *mut std::ffi::c_void {
+        self.data_backing


That's brittle. If someone calls get_allocatee_data_ptr and retains the pointer, then a call to set_alloc_backing will invalidate the pointer and we see a use-after-free.

Added HashMap for all the types of Mock so it can be stored for different interface id and event id.

darkwisebear · 2026-05-22T15:46:16Z

+        // LIMITATION: The returned pointer is intentionally dangling and must NOT be
+        // dereferenced. It is only valid as a non-null sentinel for null-checks in the
+        // mock. Any test that dereferences this pointer will trigger undefined behavior.
+        std::ptr::NonNull::<ProxyEventBase>::dangling().as_ptr()


That makes me feel uneasy... while I do get the intent, this will lead to something that knowingly returns something bad. I anyway wonder why we cannot use the classic mock approach but we provide a type that intentionally does bad things in a global way. If we had a real mock type (as e.g. made by the mockall crate), we could do such things in a controlled way locally in the test that needs it.

I'd ask you to consider using a mockall mock or give reason why this isn't appropriate.

I have added unit structs and setter functions for creating test-specific pointers. With this approach, we achieve similar per-test configurability as mockall but implemented manually, which allows us to extend it later as per requirements for coverage and unit testing scenarios.

I have tried implementing with mockall as well, but it does not work properly for our use case because we have a pointer-heavy FFI interface. When setting expectations, mockall still requires creating raw pointers, and these don't create proper type-safe instances needed by the runtime. During test execution, we faced segmentation faults due to mockall's inability to properly manage FFI pointer lifetimes and ownership semantics.
Additionally, mockall cannot handle the generic lifetime parameter in handle_container_get_at<'a>() method, resulting in compilation errors which can be overcomed as of because we have only one function but in future we get more function with generic and lifetime then this will be a blocker for us, so Mockall is not appropriate for FFI traits with generic lifetimes, complex bounds, and pointer-heavy unsafe APIs.

darkwisebear · 2026-05-22T15:51:47Z

+
+impl MockFFIBridge {
+    // The handle container is backed by a heap-allocated zeroed stub so the pointer
+    // is stable and non-dangling.  An extra `Arc` clone is intentionally forgotten


Ok, but why? isn't this something the test code may just as well do if it thinks that this is valid? (and under which circumstances is that necessary?). This strikes me again as unclean test code that makes many assumptions on how it's going to be used during testing. While this might work now, this is brittle when refactorings are being made.

Updated to the setter-based approach, which is providing necessary abstractions over FFI resource management. With this approach each test can run indivisibly and track resources.

The mock encapsulates resource lifecycle management (allocation + Drop-based cleanup) and provides type safety through generic methods like set_sample_backing<T>(). This ensures data type match FFI type at compile time.

When the FFI layer changes, we update the mock implementation once rather than modifying every test. This centralized approach is more maintainable and refactoring-safe than having scattered pointer manipulation across test files.

darkwisebear · 2026-05-22T15:54:38Z

+            drop_fn,
+        });
+        *self.alloc_size.lock().expect("Failed to lock alloc_size") =
+            std::mem::size_of::<SampleAllocateePtr<T>>();


I wonder why alloc_size is sized with the SampleAllocateePtr's size, whereas the actual data is of size T. Isn't that a mismatch..?

yes, updated with size field in BackingEntry

* 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

* Added centralized ARC in producer and consumer info

* Updated internal field to Arc * Removed Arc from runtime and using clone

* Mock FFI updated for returning pointers for proxy and skeleton * Added setter method to configure the FFI for each test * Updated Runtime test cases

bharatGoswami8 · 2026-05-31T14:50:32Z

We're not there yet. Please consider using a real mock for the bridge that implements all the trait methods and allows for setting call reactions on a per-test basis with defined values that are different from dangling pointers and other stuff that scares me.

I have added unit structs and setter functions for creating test-specific pointers. With this approach, we achieve similar per-test configurability as mockall but implemented manually, which allows us to extend it later as per requirements for coverage and unit testing scenarios.
And each test can configure the value as per test scenario which can extend as per need.
No dangling pointer now in mock ffi implementation.
With generic methods like set_sample_backing<T>() we can construct the samplePtr and allocate the as per data types at compile time rather than run time in mockall.

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 force-pushed the ffi_mock_impl_for_test branch 4 times, most recently 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 3 times, most recently 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 3 times, most recently from ae47262 to 5be1bf8 Compare May 15, 2026 12:00

darkwisebear suggested changes May 18, 2026

View reviewed changes

Comment thread score/mw/com/impl/rust/com-api/com-api-runtime-lola/consumer.rs Outdated

Comment thread score/mw/com/impl/rust/com-api/com-api-ffi-lola/bridge_ffi_mock.rs Outdated

Comment thread score/mw/com/impl/rust/com-api/com-api-ffi-lola/bridge_ffi_mock.rs Outdated

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from e78d045 to ce9b362 Compare May 20, 2026 03:38

bharatGoswami8 requested a review from darkwisebear May 20, 2026 03:43

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch 2 times, most recently from 320a521 to d1f2a25 Compare May 20, 2026 05:07

tejaswinijoshi89 approved these changes May 22, 2026

View reviewed changes

darkwisebear reviewed May 22, 2026

View reviewed changes

bharatGoswami8 added 16 commits May 30, 2026 18:05

Rust::com FFI mock implementation for unit test

35f42e4

* FFI mock api implemented for runtime unit test

Rust::com Interface trait for ffi methods

dd4b18b

Rust::com Added FFI Bridge generic parameter

c5b9bdd

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

Rust::com Unit test for consumer and producer

5ed5b67

* Added unit test using FFI mock

Rust::com move Lola FFI implementation

654b246

* Moved Lola specific FFI implementation in seperate file

Rust::com Mock implementaion updated for allocatee apis

32168ea

Rust::com HandleContainer get method update

be1ec6c

* Updated HandleContainer method for Lola and Mock

Rust::com updated SamplePtr mock implementation

a7ec233

Rust::com FFI mock and unit test for runtime

e36b50f

* Added global variable cleanup * Updated validation check on test

Rust::com Integration test fix

be0138e

Rust::com Optimized the Mock FFI

a4782dc

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

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

3a95b62

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

Rust::com FFI bridge as instance on Runtime

2b1ba41

* 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

Rust::com Optimmized the bridge clone in runtime

cddb5c2

* Added centralized ARC in producer and consumer info

Rust::com Update FFI mock

12119f3

* Updated internal field to Arc * Removed Arc from runtime and using clone

Rust::com Updated Mock FFI

b281fd1

* Mock FFI updated for returning pointers for proxy and skeleton * Added setter method to configure the FFI for each test * Updated Runtime test cases

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch from d92511c to b281fd1 Compare May 30, 2026 12:49

bharatGoswami8 requested a review from darkwisebear May 31, 2026 14:45

		// `ProxyEventBase` is a ZST opaque type, a dangling non-null pointer is the
		// canonical representation for "valid but empty" in the mock.

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bharatGoswami8 commented May 15, 2026

Uh oh!

Uh oh!

Uh oh!

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!

bharatGoswami8 May 30, 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!

bharatGoswami8 May 31, 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!

bharatGoswami8 commented May 6, 2026 •

edited

Loading

bharatGoswami8 May 30, 2026 •

edited

Loading

bharatGoswami8 May 31, 2026 •

edited

Loading