Sharing a wrapped, threadsafe C struct between threads

I'm attempting to use a Rust-wrapped C library (SOEM-rs if you're interested). This library provides a Context type that pretty much behaves as a global variable, taking many arguments by reference, as this is what the C library uses behind the scenes:

/// Provided by C library
struct ec_subt;

/// Wrapper provided by Rust crate
struct Sub(ec_subt);

impl Default for Sub {
    fn default() -> Sub {
        Sub(unsafe { std::mem::zeroed() })
    }
}

/// Provided by C library
struct ecx_context {
    sublist: *mut ec_subt,
    subcount: *mut c_int,
}

/// Wrapper provided by Rust crate
struct Context<'a> {
    context: ecx_context,
    _phantom: PhantomData<&'a ()>,
}

impl<'a> Context<'a> {
    fn new(subs: &mut [Sub], subcount: &mut c_int) -> Self {
        Self {
            context: ecx_context {
                sublist: &mut subs[0].0,
                subcount: &mut *subcount,
            },
            _phantom: PhantomData,
        }
    }

    fn send_stuff_mut(&mut self) {
        // ...
    }

    fn receive_stuff_mut(&mut self) {
        // ...
    }

    fn get_something(&self) {
        // ...
    }
}

I need to use Context in multiple threads; one which reads/writes data through a network interface and the other (main thread) with some business logic in it. Both threads need to call into the C functions which all read and write to ecx_context.

I added the following to get around the error printed by the compiler:

unsafe impl<'a> Send for Context<'a> {}
unsafe impl<'a> Sync for Context<'a> {}

The C library claims ecx_context and the rest of itself is threadsafe, and the examples included with it do spawn threads and talk to a global instance of ecx_context, so I'm inclined to believe it. There are also uses of mutexes in the C codebase to help its case.

My first question: What problems do I cause myself by implementing Send and/or Sync for Context (ne ecx_context), if we assume ecx_context is threadsafe?


As for the actual threading, here's an example using parking_lot:

fn main() {
    let mut subs: [Sub; 1] = Default::default();
    let mut subcount: c_int = Default::default();

    let c = Context::new(&mut subs, &mut subcount);
    let c = Arc::new(parking_lot::RwLock::new(c));

    let thread_c = c.clone();

    dbg!(c.read().get_something());

    let t = thread::spawn(move || {
        loop {
            {
                let mut w = thread_c.write();
                w.send_stuff_mut();
                w.receive_stuff_mut();
                dbg!(thread_c.read().get_something());
            }

            thread::sleep(Duration::from_millis(100));
        }
    });

    c.write().send_stuff_mut();
    c.write().receive_stuff_mut();

    dbg!(c.read().get_something());
}

This works (modulo any issues I don't know about with Sync/Send), however:

Question 2: if the C library really is threadsafe, is there a way to safely share Context between threads without using some kind of lock on the Rust side? I feel the "double-lock" is unnecessary but I could be wrong. I ask because the C code does it; supposedly-thread-safe-ly calling functions that modify the global ecx_context from multiple threads.

I'd also like to clean up the code somewhat; in the real code there are a lot of calls to methods on Context so it would be nice to reduce all the read()s and write()s from parking_lot.

Perhaps I'm barking up the wrong tree and there's a completely different, better way to model this entire thing in Rust?

And finally Question 3: Would Pin help anything here? I've read a few bits about it but I'm not clear if it would be useful in my case.

Full example code on the playground.

For completeness' sake, the IRL code I'm using at the moment is here. Notice the commented out thread::spawn which is causing me problems there as I'm not using parking_lot::RwLock (hoping to find a better solution).

If you can assume ecx_context is indeed threadsafe, none.

The Sync trait effectively means I'm allowed to call &self methods from multiple threads and Send means I can move ownership of the ecx_context to other threads safely (i.e. no thread-local variables are used).

You just need to make sure all methods take &self and the Sync impl will take care of the rest. You will still need to wrap your ecx_context in an Arc though to make sure the object doesn't get freed while something is still using it.

One massive problem you are going to have is that Context::new(&mut subs, &mut subcount) constructor. Your Context::new() arguments don't have a 'a lifetime or anything else to restrict its scope, so 'a is inferred to be 'static (I can tell because std::thread::spawn() doesn't let you pass non-'static references to the spawned thread). This means I could easily trigger a use-after-free by doing this:

fn create_context() -> Context {
  let mut subs = [Sub::default()];
  let mut subcount: 0;
  Context::new(&mut subs, &mut subcount)
}

In which case, I would probably change Context to carry the subs and subcount along with it. The ecx_context contains pointers to them, so we need to make sure they can't be moved after creating the ecx_context. Maybe something like this?

struct Context {
  ctx: ecx_context,
  // Note: the ctx field has mutable references to these two so we 
  // to aren't allowed touch them. That also means you can't
  // derive `Debug`.
  subs: Box<[Sub; 1]>,
  sub_count: Box<c_int>,  
}

impl Context {
  fn new() -> Arc<Context> {
    let subs = Box::new([Sub::default()]);
    let sub_count = Box::new(1);
  
    unsafe {
      // Note: You can think of this as moving ownership of subs and the sub_count
      // into the context.
      let ctx = native_function_to_create_ctx(subs.as_mut_ptr(), &mut sub_count);
      Arc::new(Context { ctx, subs, sub_count })
    }
  }
}

impl Drop for Context {
  fn drop(&mut self) {
    unsafe { native_function_to_free_ctx(&mut self.ctx); }
    // this is now legal
    println!("Subs: {:?}", self.subs);
  }
}

Obviously, I haven't tested this code and it would require going over with a fine-toothed comb to make sure our unsafe Rust matches up with the C code's expectations, but you get the idea.

You also want to avoid having lifetimes in your top-level Context type anyway because they make things hard to use. That 'a lifetime will be infectious and mean anything you want to put the Context in will also need to be 'a. It also means you won't be able to pass it to another thread (possible use-after-frees) and storing it in a Box or Vec won't work the way you think.

I don't think so because Pin is mainly intended for Rust types that contain internal references. Your ecx_context will already be on the heap and never move because you wrapped it in an Arc, so you don't really gain much here.

I could be wrong here. The precise semantics around Pin are quite subtle.

4 Likes

This is correct. Pin is not useful here.

1 Like

Thank you both very much for taking the time to answer, this has put a lot of stuff in my head to rest.

Unfortunately I have to use &mut self in many places as it modifies ecx_context. Is this solved by using an RwLock (or other mutex), or are there other issues to consider? I'm somewhat new to how Rust makes memory and threads interact so apologies if this is a basic question.

I tried that in, with the same reasoning even. Good to know I was half on the right track! I think this is why I was asking about Pin. That said, if the Context struct is moved, won't that invalidate the pointers in ecx_context that point to its members?

Agreed, the lifetime was just how the library was written. I've felt since the start it should probably own all the stuff passed into it even though ecx_context is all references so this is good confirmation.

If the ecx_context is doing locking internally your methods can take a shared &self instead of &mut self. Sometimes it's more helpful to think of & and &mut as "shared" and "unique" references.

Of course, you can wrap the entire thing in a RwLock, but I feel like adding a second layer of locks to something that already uses locks may be overkill.

That's why I suggested boxing the values. That way you can move Context around as much as you want and you'll just be moving the Box, not the subs and sub_count values themselves.

This is mainly a question of coding styles and ergonomics. It is more user-friendly for a long-lived object like Context to own everything because owned values can easily be moved around and their lifetime isn't tied to a single stack frame (e.g. imagine creating the Context in one thread and passing it to another).

That said, sometimes the C code is structured in a way that it only uses borrowed data and it feels more convenient to not do the boxed subs thing, in which case lifetimes are the perfect solution.

You've broadened my thinking of Rust with this, thank you :slight_smile:

Sorry, missed the Boxes in your examples - good point.

I actually decided to run SOEM through c2rust and do everything the really hard but pure Rust way. The C code doesn't fit well into Rust's model IMO, so what better solution than to just remove the C and refactor it on the way :stuck_out_tongue:

Thanks again for your time, this has been a great learning experience!

2 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.