Add feat. 'lock': Thread-safe API

[mclrc]

After some back and forth with different approaches, this is what I came up with regarding #8. There are some minor things to iron out, but I think this should address the issue well enough.

Basically, when the feature 'lock' is enabled on the main crate, a new attribute macro impl_for is run on all the wrapper functions. It simply generates an impl block for SpiceLock, which is the guard singleton we discussed, containing an associated function that is identical to the wrapper the macro was executed on, with the exception of an added &self arg. This function simply calls the original wrapper with the passed arguments.

I put this attribute on all wrappers, except the ones in raw for which neat versions exist. In those cases, I only put it on the neat version.

SpiceLock is !Sync + Send, meaning it can only be shared between threads when wrapped in a Mutex. You can find an example of the API and multi-threaded usage in the tests I wrote. Lock-specific testing is a bit light right now, I'll write some more tests tomorrow.

The only other issue I see right now is that the usual API is still available to consumers when the feature is enabled. For the direct C bindings, I think this is actually a good thing, as they are the only way you can use SPICE functions which don't yet have wrappers. And they are marked unsafe anyway. However, the neat and raw APIs should probably be hidden to prevent accidental calls to non-guarded functions.

But, since the tests in rust-spice are integration tests, you cannot conditionally expose items to them using #[cfg(test)], which means that they would all then break or have to be excluded with the feature enabled. Or they could pretty trivially be converted to unit tests. What are your thoughts on this?

Only conditionally exposing the usual API also breaks the doc test in the README. I don't see a way to guard against this currently since the file is in straight markdown format.

Please don't hesitate to criticize and request whatever changes you would like to see! I'm eager to learn! :)
@mkalte666 I'd love to hear your thoughts as well, since you originally raised the issue.

Cheers, a wonderful day to you!

[GregoireHENRY]

Hi @pixldemon, congratulations! It seems you have achieved to get rust-spice to work safely with multi-threads for some at least the str2et function. I also note that you respected the requirement of being optional feature without disturbing other users.
I'm also curious to see what @mkalte666 thinks of this.
TBH I don't understand why the lock prevents UB (see comment in test multiple_threads). I'm actually wondering if it is correctly working in parallel. Thus, I would appreciate to see a benchmark to attest this is indeed doing parallel computation. You would be okay to do that to show off the feature?

[mkalte666]

This sounds great, im gonna try to take a look at this on the weekend! Hopefully... My schedule is pretty full at the moment xX

@GregoireHENRY the reason that locking prevents UB is that, in a single threaded env, most operations that are plainly wrong are actually nicely caught and handled by spice itself - like unloading a kernel and then trying to use it. Spice has nice error handling down the line from this, either with aborting, setting a global error flag, etc etc.

The library does, however, have a lot of global state.
This is what is the UB here - the (mutable) access to the global state from two threads at the same time.

Locking access to spice to one thread at the a does prevent this - you now only can have logic errors- which are reported nicely - or stuff like deadlocks and the like.
Of course one could argue that this reduces the benefit of multithreading - you cannot parallelize calculations done in spice, because the spice functions access global state and are not pure.

However, if you do a lot of stuff yourself?
This was my use case; i had a 6000+ sat TLE sim running in one thread and wanted to still have a responsive gui in another, while still somehow being able to access time conversion functions.
So this brought significant benefits.

I skimmed the PR quick and noticed this: One thing i think you might wanna do is mark all the functions as &mut self. You can, think, even put something !Sync into an arc and access is globally if it is non-mutable, cause the functions are supposed to be read only. You can, IIRC, either have one mutable ref, or multiple non-mutable ones. My rust might be a bit, well, rusty here, its been a while since i dealt with this.
EDIT: forget this, arc requires Sync thus this doesn't work. Im so gonna steal that tagging for my spice wrapper lol.

[mclrc]

Thank you!

Well, it's not actually parallel computation. As the underlying library is inherently incapable of this, I am pretty sure it would require multiple instances of CSPICE to achieve. The purpose of the lock is to enable safe use of CSPICE functions in multiple threads by guaranteeing that only one thread at a time can actually call them. This is how I understood the original issue, and it is also what @mkalte666's quick out-of-crate solution accomplished, albeit with the potential of runtime crashes.

This is what this PR provides. If you ran the same str2et calls in the test without the lock guarding them through the raw API, you will find that the program crashes every time. While forcing CSPICE functions to execute sequentially isn't as useful as actually concurrent SPICE calls would be, I think some mechanism like this is a necessity to confidently use CSPICE from multiple threads, especially considering Rust's ethos of "Fearless concurrency."

If a program spends the majority of it's time using SPICE functions, like the benchmark you suggested probably would, a lock like this won't help speed it up. However, if a program is spending a lot of time performing calculations using SPICE data which benefit from parallelization, it would make it possible to guarantee that each thread can load and write SPICE data safely when needed.
EDIT: @mkalte666 seems to have replied at the same time as me with an example of this. Cheers!

I hope I could clarify! A great day to you! :)

[mclrc]

Alright, the raw and neat APIs are now hidden with the feature enabled. Because this breaks doc and integration tests, I conditionally disabled them as well.

I also merged the change you made to the reacquire unit test, and used doc_cfg to have SpiceLock and it's methods documented on docs.rs.

This is kind of a massive PR, sorry about that. I'd be happy to explain or make any changes you want. Please don't merge if it's not to your liking.

In any case, I'd love to hear what you think!

Have a nice day! :)

[GregoireHENRY]

Nifty! What a work! 👏

Firstly, thanks for the clarification and further explanations you both wrote.

Secondly, nicely done catching up my mistake of committing change to reacquire at the wrong place, should have done that on your fork.

And then, congrats on hiding the modules depending on the activation of the lock feature. I like the PR and it looks helpful to your use case. I'm wondering about the position of the new section in README, maybe further down as the "In development" should be just after "In action" to highlight the benefit of the wrapper. But this can be revised later.

One thing to note with the section addition in the README though, is that testing the code inside it will fail if the lock option is disabled. But this is not a big deal, we can figure that out later.

That's a nice work!