Thread Safety

[mkalte666]

The c library is, as far as i know, not thread safe. Thus using anything currently provided by this library (be it wrapped or not) also is not. However, functions like spice::str2et can be called from different threads. That breaks stuff.

I am, for now, using this ugly ugly ugly thing to keep stuff from breaking for me:

https://github.com/mkalte666/satwatch/blob/2545d4d980104fe35527f14b8494385258a157b9/src/libspace/src/spice_lock.rs#L1

I think it would be nice to be forced to call something like spice::new() returning an object that provides the whole api. Users can then wrap it in an arc to keep being able to put stuff into other threads, being forced to use proper synchronization.

It seems like a bit of a niche case, but at least making sure users don't end up with weird errors cause they don't know they cant access stuff from other threads seems like a good idea to me. I might be missing something somewhere, so if thats the case, id be happy to be corrected and have that pointed out for me.

In any case, have a good weekend :D

[mclrc]

I like this idea. If it's too big of an API change you could lock it behind a feature flag and warn about thread safety in the docs. When the feature is enabled, cspice_proc! could generate methods and then put those in a !Sync singleton struct. I'd like to take a crack at it, @GregoireHENRY would you be interested in a PR like this?

[GregoireHENRY]

Hi @mkalte666 and @pixldemon, Thanks @pixldemon for bringing up this discussion, and sorry @mkalte666 for having not replied earlier.

Thread safety is a big topic for SPICE, and indeed SPICE in general (not only the C API, because this is due to how kernels work) is not thread safe (see 1, and 2). The NAIF team works on a SPICE 2.0, a new version of the toolkit implementing thread safe toolkit (2).

Besides, I want to point out the ANISE project which studies the feasibility of developing a thread-safe API as a modern replacement of NAIF SPICE file.

What about solutions for rigth now? We can take the experience of the python wrapper SpiceyPy, that has been around for longer. For that, I want to point you out to the contributing guide of SpiceyPy which mentions the philosophy of the wrapper. It says that the objective is to provide a pythonic interface to CSPICE and not much else (3). Also, small differences are expected between CSPICE and SpiceyPy API but this only for simpler interaction benefiting from Python high-level style (3). Hence, I think the same should apply to this wrapper and should be kept in mind.

Now let's see if addind thread safety to some selected functions is in the scope of the wrapper. Still observing decisions made on the python wrapper, I can see this issue which opened the topic of discussion for multi-threading, and there is this on-going pull request trying to find a solution to the non thread safe problem. They do mention locking the access to the SPICE resources, which looks do like what you suggest. The discussion is quite long and we should really benefit from reading it entirely. For example, I see later in the discussion that just locking resources between load and unload is not safe enough and produced error in some case. Furthermore, the latest comment on this discussion seems not positive for this feature.

I'll read the multi-thread PR of SpiceyPy in more detail a bit later, also read the code and tests that they tried and come back to you later. Let me know if you have done the same and see the things differently. Have a great day!

[GregoireHENRY]

I like this idea. If it's too big of an API change you could lock it behind a feature flag and warn about thread safety in the docs. When the feature is enabled, cspice_proc! could generate methods and then put those in a !Sync singleton struct. I'd like to take a crack at it, @GregoireHENRY would you be interested in a PR like this?

To answer you @pixldemon, I'm very interested and curious to see if it could work in Rust where Python (looks like having) failed. I want to believe this can be possible. So I'm okay to help on PR that you would open on that. The only requirement is that it shall not change, in any way, the experience of a user of the wrapper, expecting by default the behaviour of CSPICE.

[mclrc]

Wonderful, I'll probably start working on it today! :)

As for the discussions you linked, if I understand the SpiceyPy PR correctly (I have basically no experience with Python multi-threading), it wraps all functions in a decorator that acquires and releases the lock on every function call, meaning:

1. If enabled, it forces the overhead of locking and releasing every single function call
2. Because the lock is acquired and released with every function call, it only ever guards one call at a time, which means another thread can acquire the lock in between calls and mess with what the first thread was doing.

We can solve both calling each function through a lock instead of acquiring it within every call. This should be relatively straightforward. The lock could then be kept by one thread for the entirety of a critical section, with no possibility of another thread messing with it, and with an arbitrary number of SPICE function calls and just one acquire/release.

This does not solve the problem of one thread requiring a previously loaded kernel or variable that another thread unloaded or similar, but I think it's more than reasonable to expect a consumer to prevent this from happening. Like the people in the SpiceyPy discussion, I don't think there is a trivial solution to this without complicating the internals and API significantly. However, if what a thread does within a critical section is relatively self contained, I think it shouldn't be too much of an issue. For things like error handling, each thread could simply configure it's desired behavior at the start of each critical section with negligible overhead.

Let me know if I overlooked or misunderstood something, which is very possible. A great day to you as well, and thanks for the quick and insightful reply!

[mkalte666]

Thanks for getting back to this!

A full thread safe (as in getting rid of the massive global state) API v2 would certainly be preferable, however for me and my projects (that, admittedly, i don't really have much time to work on right now), some kind of guaranty that i cannot access the API from two places at the same time would be enough.

That can, i think, be achived by wrapping the safe functions into a singleton struct impl that is not send+sync, so the only way to get (mutable) state across threads would be with an arc+mutex.

This does not, of course, protect from doing weird things in place a and then being hit by it in thread b; however, the API does not guarantee this anyway, so unloading kernels and then trying to do stuff with them is very much a problem even single threaded right now.

These are catch able (by wrapping a result type, just using spices error handling, ...) logic errors though, while accessing stuff from two threads simultaneously might end up as UB.

If you are interested, i have started working on my own wrapper that everyone can feel free to grab ideas from, as i am not sure if i will ever have the time to bring it to completion. I will continue tinkering on it though, its a good unsafe-rust training :D https://github.com/mkalte666/naifspice-rs Now that im talking about it, for the above-mentioned guarantee id have to mark every function (&mut self) of i think, woops :D As id said, not something for production use right now, just my tinkering

[GregoireHENRY]

Closed with #10