feat: Replace Arc with Rc to fix clippy lint

[sjackman]

• Replace Arc with Rc in StarReference
• Remove unused unsafe impl Sync for InnerStarReference and unsafe impl Send for StarRef
• Needed to bump RUST_VERSION from 1.65.0 to 1.79.0

The StarReference containing the Rc is cloned in the main thread, the clone is sent to the worker thread, and the worker thread returns the clone back to the main thread, which destructs it and its Rc, so a Rc is sufficient and an Arc is not needed.
https://github.com/10XDev/cellranger/blob/ca2035689090380a46c3f19b9f5a32a375e111c4/lib/rust/cr_lib/src/stages/align_and_count.rs#L814

Fix the clippy lint:

Error: error: usage of an `Arc` that is not `Send` and `Sync`
  --> src/lib.rs:66:20
   |
66 |             inner: Arc::new(inner),
   |                    ^^^^^^^^^^^^^^^
   |
   = note: `Arc<InnerStarReference>` is not `Send` and `Sync` as `InnerStarReference` is not `Send`
   = help: if the `Arc` will not used be across threads replace it with an `Rc`
   = help: otherwise make `InnerStarReference` `Send` and `Sync` or consider a wrapper type such as `Mutex`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
   = note: `-D clippy::arc-with-non-send-sync` implied by `-D clippy::suspicious`
   = help: to override `-D clippy::suspicious` add `#[allow(clippy::arc_with_non_send_sync)]`

See https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync

[macklin-10x]

I'm rather surprised that the compiler is OK with this.

[sjackman]

I'm rather surprised that the compiler is OK with this.

Took me a minute to figure out what was going on, but the Rc is cloned and dropped in the main thread, so there's no asynchronous access to its reference counter.

[macklin-10x]

Like, yes, that's correct, but the compiler has no way to track that; the mere fact that it isn't Send should be enough to make this not work. Is the multithreading aspect handled in C++ code, and that's why?

[macklin-10x]

I think the correct fix there is to unsafely impl Send for InnerStarReference, assuming that is true, and keep the Arc.

The only reason it wouldn't be true is if it matters which thread the destructor for InnerStarReference runs on:

impl Drop for InnerStarReference {
    fn drop(&mut self) {
        unsafe {
            bindings::destroy_ref(self.reference);
        }
    }
}

If that does need to run on the same thread that instantiated it, then unsafe impl Send is a dangerous idea.

It kind of begs the question: if there's no need to synchronize access to BindRef, why are we using Arc in the first place? Instead we should instantiate InnerStarReference in some kind of container that never leaves the invoking thread, and hand out &InnerStarReference, thus guaranteeing that the bindings are never destructed on the wrong thread.

[macklin-10x]

The parallelism in cellranger is using crossbeam scoped threads, so sharing by reference should work.

[sjackman]

Thanks for the explanation, Chris! Closing this PR, superseded by PR

• feat: impl Send for InnerStarReference for clippy #80

[adam-azarchs]

If there isn't logically ever more than one copy of it, why not just use a Box?

[macklin-10x]

Boxing doesn't achieve anything here; the reference to it needs to be shared between threads, and Box is still single owner.

[adam-azarchs]

Well, then it sounds like we do, in fact, need Arc.

[macklin-10x]

The right long-term solution is still to share a scoped immutable reference, but continuing to use Arc is fine for now.