no_std support, more minimal and less intrusive version

[Alexis211]

This PR proposes a non-invasive, relatively minimal implementation of no_std support for the arc-swap crate. The aim of this PR is to enable no_std without much effort for specific use-cases such as embedded or os dev, which are often already using a nightly compiler anyways, without disturbing the existing functionning of the crate. I saw the previous PR for no_std support (#56) which was not merged due to lack of activity. If possible I'd like to do the necessary work so that we can have no_std support in arc-swap soon.

• This PR is minimally invaisve: if the no-std feature is not enabled then the build is pretty much exactly the same. The crate could successfully be compiled with Rust as low as v1.38.0 (same as before, see below).

• Compiling with the no-std feature requires a nightly Rust compiler, due to the use of unstable features thread_local and lazy_cell. No additionnal dependencies are added. Suppor for RwLock is dropped in no_std mode.

• This PR does not require additionnal OS dependencies and does not introducing more locking or synchronization points (it doesn't use the static_init crate as in no_std #56). Due to lesser requirements, it is probably usefull on more platforms than no_std #56, as long as support for compiler-generated thread local variables is available. FWIW, the tls-mode=emulated makes it quite easy to add support for these thread-local variables on a variety of platforms even without OS/kernel support, without having to delve into the intricacies of linkers and symbol relocation (example in C from the Clang source code).

To answer questions from the other thread (PR #56):

First of all, how lock-free is the library for thread-local storage and initialization? I think it would be OK for it to have some locks on the first access in each thread, or on thread creation. But it would kind of beat the whole purpose of arc-swap if there was a lock in each access. Did you have a chance to study the documentation or implementation?

In this PR, we are using only the experimental #[thread_local] tag of the nightly compiler. This compiles down to LLVM thread local primitives, which are supposedly one of the fastest possible underlying implementation for std's thread_local! macro, so no performance penalty on that side is to be expected. Custom implementations can also be made easily without locks. The thread local variable itself that is used by arc-swap is a LazyCell which is not Sync, so there is no mutex or synchronizaton penalty on initialization.

Without enabling the new feature, all previous features need to compile as previously. Specifically, with no features enabled, it must be possible to compile even with 1.31.0.

This is the case with this PR, albeit the lowest supported version is 1.38.0, which is not related to changes in this PR. Versions <=1.37.0 failed with the following error, which was already the case on the master branch:

error: failed to parse lock file at: /home/lx/dev/crates/arc-swap/Cargo.lock

Caused by:
  invalid serialized PackageId for key `package.dependencies`

I believe this new feature doesn't have to be incompatible with the weak feature.

Not an issue in this PR.

I also believe the new feature should be compatible with stable rust, not work only on nightly.

Unfortunately, this is not doable easily in the current state of the Rust ecosystem, at least not with a lot of additional effort. This PR is a minimal try to land no_std support in the arc-swap crate as simply as possible, so it naturally comes with some restriction. I tried to have as little restrictions as possible, and basically there are only two conditions for this to be used: use a nightly compiler, and have support for compiler-generated thread local variables.

I'd be glad to have your comments and advice on how we can make this into something that can be merged into arc-swap.

[vorner]

Reading through the changes, I must say I kind of like them. The overall feeling is it is kind of elegant and nice.

I do have few details to discuss, though:

• I think the CI needs to be adjusted a bit. Currently, it tries to run the no-std in stable / beta, which fails.
• How far away from stabilization are the features you use? Is it worth waiting for them, or can we rely on them not changing much?
• Eventually, it would be nice to use these in normal std build too, if they prove faster. In that regard, the no-std feature flag seems a bit misleading. Maybe something like lazy-cell feature instead? Nevertheless, as long as this is a nightly-only feature, it probably should be marked as such ‒ something like experimental-lazy-cell, so people know the stability guarantee doesn't hold.
• I think adjusting documentation would also be in place.

As for the old build… I think you can still build the crate if you delete the lock file. The lock file is not used when the crate is just a dependency.

[Alexis211]

How far away from stabilization are the features you use? Is it worth waiting for them, or can we rely on them not changing much?

The thread_local feature has been in Nightly for a very long time, it doesn't look like it will be ready for stabilization anytime soon, but it also doesn't look like it is changing a lot. I think this is the main feature that is gating no_std support in our case. See rust-lang/rust#29594

The lazy_cell feature is much more recent (<1 year, see rust-lang/rust#109736), and it looks like the design is still under debate, so maybe it's not a so good idea after all to rely on it. I pushed a new version that achieves the same result using RefCell<Option<_>> instead of LazyCell<_>, it's just a bit less elegant because we need explicit testing for initialization in LocalNode::with, but it works. This should have near-zero impact on performance.

Eventually, it would be nice to use these in normal std build too, if they prove faster. In that regard, the no-std feature flag seems a bit misleading. Maybe something like lazy-cell feature instead? Nevertheless, as long as this is a nightly-only feature, it probably should be marked as such ‒ something like experimental-lazy-cell, so people know the stability guarantee doesn't hold.

I pushed a commit that renames the feature flag experimental-thread-local, but then there is the question of support for std::sync::RwLock. Currently it is disabled when experimental-thread-local is enabled, but I was thinking maybe we could have a feature rwlock that enables the std::sync::RwLock support, and set it as a default feature. What do you think?

I think the CI needs to be adjusted a bit. Currently, it tries to run the no-std in stable / beta, which fails.

I pushed a commit that replaces all occurences of --all-features in the CI scripts with an explcit list of features without experimental-thread-support.

I think adjusting documentation would also be in place.

Yes, I didn't think of it. I'll do this last, when the other questions are resolved.

[vorner]

Nice :-)

Few more things, though…

I pushed a commit that replaces all occurences of --all-features in the CI scripts with an explcit list of features without experimental-thread-support.

I think we also should have (at least one) test where it does compile with this one.

As for the rw_lock. It should be enabled only under the internal-test-strategies feature flag (if it is compiled without that feature, then it's a bug… but I believe it's at least not exported in that case), which is not shown anywhere in the documentation and it very clearly says not to be used from outside in the Cargo.toml, that there are no stability guarantees for that one. I'd say we don't need another feature flag for that one, but maybe having an explicit error message if both feature flags are enabled at once might be nice.

[Alexis211]

I think I fixed pretty much everything and added some documentation. Concerning the rw-lock thing, could you check that I did what you were expecting? Especially the conditional compilation in src/strategy/mod.rs.

Thanks :)

[vorner]

Sorry for the delay on my end; this looks good. But the CI tests are currently failing due to some problem in proc-macro2 vs. newest nightly compiler. It is already resolved on master, so a rebase might help.

Also, when I do cargo test --features experimental-thread-local (on nightly compiler, with dependencies updated), it fails. It seems the mod tests sections weren't updated to use alloc::whatever instead of std::whatever and can't find things like Vec or String.

[Alexis211]

I fixed compilation of the tests when running cargo test --features experimental-thread-local, but unfortunately the tests fail or hang. It looks like the thread local variable is not working correctly, for instance if I focus on the rcu test, I can get outputs like this:

$ cargo test --features experimental-thread-local rcu
    Finished test [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/debug/deps/arc_swap-834f36c9a23df4c3)

running 1 test
test tests_default::rcu ... FAILED

failures:

---- tests_default::rcu stdout ----
thread '<unnamed>' panicked at src/debt/list.rs:253:47:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at src/debt/list.rs:253:47:
already borrowed: BorrowMutError
thread '<unnamed>' panicked at src/debt/list.rs:253:47:
already borrowed: BorrowMutError
thread '<unnamed>' panicked at src/debt/list.rs:253:47:
already borrowed: BorrowMutError
thread '<unnamed>' panicked at src/debt/list.rs:253:47:
already borrowed: BorrowMutError
thread 'tests_default::rcu' panicked at src/lib.rs:1236:1:
called `Result::unwrap()` on an `Err` value: Any { .. }


failures:
    tests_default::rcu

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 31 filtered out; finished in 0.00s

error: test failed, to rerun pass `--lib`

Looking at a stack trace, it looks like there is a recursive function that calls LocalNode::with within itself, leading to double-borrowing. I'll try to investigate if I have time.

[Alexis211]

Alright, I fixed the tests by using OnceCell instead of RefCell. All tests are working now with --feature experimental-thread-local, I just had to disable the rcu_panic test because panic::catch_unwind is not available in no-std mode.

[vorner]

OK.

Before I go ahead and merge it, do you want to do some cleanup of the history? Because there's a lot of „changes due to review“ commits (which are kind of annoying in future browsing of the history, bisects, etc) instead some kind of per-topic commits. I can either squash it as a whole to a single commit, or you can sort it into something „nice“.

[Alexis211]

If you don't want to keep the current history, I think it's fine to just squash everything in a single commit.

[vorner]

Thank you, I've done a release.