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.

* 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

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

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

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?

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?

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.

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.

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.

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.

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.

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

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

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 from ce9b362 to 320a521 Compare May 20, 2026 04:16

Rust::com Update FFI mock

d1f2a25

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

bharatGoswami8 force-pushed the ffi_mock_impl_for_test branch 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

		// `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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bharatGoswami8 commented May 6, 2026 •

edited

Loading