Thread safety

[pcercuei]

The readme states that "the contexts and devices in the underlying libiio are not thread safe."

They actually are, though. You can't obviously access some resources at the same time (e.g. trying to refill a buffer in two different threads at the same time), but you can read/write attributes in one thread and manipulate the buffer in another thread, it works just fine.

[tomstokes]

Analog doesn't guarantee thread-safety of the libiio functions, even if some of them happen to be usable across threads in their current implementation ( https://ez.analog.com/linux-software-drivers/f/q-a/90775/safe-to-assume-that-libiio-calls-are-thread-safe/197587#197587 )

The unreleased master branch exposes iio_context_clone to allow cloning contexts across threads: 14de9c2#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

[pcercuei]

Thread safety is guaranteed in the libiio functions. Libiio has been developed from the ground up with the idea that it should be thread-safe.

[fpagliughi]

@pcercuei Thanks for bringing this up! I have been in the same boat as @tomstokes; after reading some old blog posts, etc, was under the assumption that the library was not thread safe.

The problem with Rust (oops... I mean the great thing about Rust) is that it requires you to be completely explicit about things in regard to thread safety. So the statement "you can't access ... some resources at the same time" gets a little tricky. But I will definitely look into it for the next version.

[fpagliughi]

Having thought about this for a while, I'll likely split the difference...

Having believed that the contexts were not thread-safe, I made all the Rust objects !Send and !Sync, meaning that individual objects can not be shared across threads (!Sync) and they can not even be moved from one thread to another (!Send). This means that all objects derived from a context are forced to live in the same thread as that context.

Ha! Try that in C. 😄

But, with this new information, I'll start relaxing those constraints, starting by making Device movable across threads (i.e. Send). This would mean that the Context needs to be shared across those threads, and thus needs to be Sync.

As a first step, it would mean that a Device still couldn't be shared across threads, and that a Buffer object would still need to live in the same thread as the Device. With some further information about what's thread safe and not, I assume that this could also be relaxed.

But certainly it sounds like the Buffer::refill() function should take a mutable reference to self which would prevent it from being called by two threads simultaneously even if it were shared between them.

I'll take these one step at a time, with a release in between to try and get it right.

...and I'll update the README.

[1NFR4R3D]

Hi! I was wondering what the status on this is.
If thread safety isn't a high priority for you, I don't mind working on it myself and submitting a PR.

[fpagliughi]

If thread safety isn't a high priority for you, ...

Ha. I'm not sure if you got this thread backwards. The crate wraps the C libiio library. Online there are some older documents that would let you believe that the C lib is not thread safe, so when I created this Rust wrapper library, I went to total thread safety, not allowing any of the C types to be shared or even sent across thread boundaries.

@pcercuei is one of the main authors of the C lib and kindly let us know that out assumption was incorrect, and that the C lib is mostly thread safe.

So, it turned out that the Rust library was too thread safe. A number of objects could be sent or shared across threads.

Much of the work to loosen the onerous constraints was done in v0.5.0. From the CHANGELOG:

• Started loosening thread safety restrictions:
  • The Context is now Send and Sync. Internally it has canverted to using an Arc instead of an Rc to track it's internal data.
  • The Device is now Send.
  • For high performance with multiple device, though, it's still recommended to used fully-cloned contexts for each device
  • For now, Channel and Buffer objects are still !Send and !Sync. So they should live in the same thread as their channel.

I left this open knowing that there was probably some more that could be done to remove thread restrictions (like some type could possibly be Sync that are not currently defined as such), but this would require a pretty deep dive into the C code to make these determinations.

As it stands now, I think the library is pretty usable without risking thread safety.

[fpagliughi]

Are there any current constraints that can't be worked around?

[1NFR4R3D]

Would you consider making more types (especially Device) Sync by wrapping their members in an Arc<Mutex<T>> in scope for this library? That is the only constraint I have. I'm sure I can work around that, but I feel like this work could help others as well.

P.S. I might be naively overlooking some things here, please tell me if I am

[fpagliughi]

I'm not sure that I would want to do that as it would be a performance hit on existing users, to lock and unlock on every device access.

And by the comments above, I'm not sure if/when that is necessary. Looking at the C code, though, it seems that maybe different attributes could be updated simultaneously by different threads, but updating the same attribute would be problematic.

How many threads do you imagine you would want accessing a device? Certainly you could wrap a Device in an Arc, but alternately you could clone the context and create a separate Device struct for each thread.

[1NFR4R3D]

Sorry for being late in getting back to you.

I'm not sure that I would want to do that as it would be a performance hit on existing users, to lock and unlock on every device access.

That makes complete sense. Should have thought of that.
I can always do the wrapping in my implementation.

My use case gets a bit complicated because I expose a dbus interface which performs a read or write on every dbus interaction. After some thought (and a break, and a good night's sleep), I can always cache the reads and only update my cached values at (or above) a sampling rate. That will solve most of my possible problems.

P.S. thank you for the work around the crate!