Clean up Feature Flags API, add support for scoped feature tests by henrikhorluck · Pull Request #9881 · fish-shell/fish-shell

henrikhorluck · 2023-07-09T16:30:39Z

Previously you needed unsafe to set feature flags from Rust. This also allows
scoped feature tests that makes testing feature flags thread-safe.

Note for posterity: you only needed unsafe since the set/set_from_string took &mut self when they could take &self since the feature flags are stored in atomics

krobelus · 2023-07-09T19:37:29Z

 unixstring = "0.2.7"
 widestring = "1.0.2"
 rand_pcg = "0.3.1"
+parking_lot = "0.12.1"


parking_lot is a drop-in replacement for the standard sync primitives?
We should not have multiple ways of doing the same thing without a good reason.

The std::sync::RwLock does not support upgradeable readers/downgradeable writer, which is necessary for scoped_test to atomically change from a writer to a reader for the test to actually read the feature value. Sadly the diff for adding scoped_test got mangled¹. This is getting added to avoid race-conditions as tests in Rust are multi-threaded by default, unlike the C++ ones, see #9854 (comment) for where this will be used.

We should be able to use parking_lot as a dev-dependency and use std::sync:RwLock otherwise, but I don't see the point in doing so as it just causes unnecessary cfg()'s ,and parking_lot::RwLock is generally faster afaik https://crates.io/crates/parking_lot

Footnotes

Overall diff still mangled, but the individual commit is much nicer https://github.com/fish-shell/fish-shell/pull/9881/commits/d9857f28cbc98e2fbbc047de3402941db0465506 ↩

can we tell Rust to not run this test in parallel to others?
In Go you have to explicitly enable parallelism for tests..

That requires either cargo test -- --test-threads=1 or using a separate test-runner like https://nexte.st and using test groups https://nexte.st/book/test-groups.html

krobelus · 2023-07-09T19:37:30Z


 /// The singleton shared feature set.
-static mut global_features: Features = Features::new();
+static FEATURES: RwLock<Features> = RwLock::new(Features::new());


I don't see a reason for using locking instead of atomic bools.
(Also our current stance is to use the simpler Mutex unless we really need concurrent multiple readers)

IIRC feature flags are set early on during startup and never changed.
From that PoV, atomic bools make most sense because they are usually zero-overhead

TBH this change can be #[cfg(test)]'d. I don't see the point arguing about the performance for functions that are called very rarely. This is a RwLock since we need to guarantee that a test that's dependant on one particular feature flag actually has that value during the test. AtomicBool does not work for that, and neither does a Mutex, unless I am misunderstanding how you are suggesting to use the mutex.

This also cleans up and removes unnecessary usage of FFI-oriented `feature_metadata_t`, which is only used from Rust code after `builtins/status` was ported.

ridiculousfish · 2023-07-10T01:37:38Z

The cleanup commit looks good, thank you!

I agree with krobelus here: I am generally suspicious of read-write locks, and I would prefer for the common case to be a simple read of a global boolean, and not have the design be overly influenced by the needs of the tests.

How about introducing a thread-local Option<Features> which, if set, is used in preference to the global one?

The advantage is that Features can continue to use atomics, and the code to check the thread-local value can be made conditional on whether we are running the tests. This seems like a nice way to maintain test parallelism without overly distorting the design of Features.

henrikhorluck · 2023-07-10T04:58:09Z

I agree with krobelus here: I am generally suspicious of read-write locks, and I would prefer for the common case to be a simple read of a global boolean, and not have the design be overly influenced by the needs of the tests.

How about introducing a thread-local Option<Features> which, if set, is used in preference to the global one?

The advantage is that Features can continue to use atomics, and the code to check the thread-local value can be made conditional on whether we are running the tests. This seems like a nice way to maintain test parallelism without overly distorting the design of Features.

I don't think it matters enough to argue about. Changed it to a thread-local

This also allows scoped feature tests that makes testing feature flags thread-safe. As in you can guarantee that the test actually has the correct feature flag value, regardless of which other tests are running in parallell.

henrikhorluck · 2023-07-10T05:23:02Z

+    #[cfg(not(any(test, feature = "fish-ffi-tests")))]
+    {
+        FEATURES.set_from_string(wstr)
+    }


BTW I don't think this cfg is built in CI, as if so it should've failed in one of my pushes

mqudsi

Instead of using parking_lot + RwLock, we can use an atomic counter and a mutex, no?

I feel like features are such a fringe feature that they shouldn't impose such a burden to the rest of the code. The use of unsafe for features support isn't really the end of the world.

henrikhorluck · 2023-07-10T20:02:31Z

Instead of using parking_lot + RwLock, we can use an atomic counter and a mutex, no?

I really do not see any point in rolling our own atomic/sync container implementation just for features, this is not important for performance and I want correctness without having to think about it.

I feel like features are such a fringe feature that they shouldn't impose such a burden to the rest of the code.

By using scoped_test everywhere instead of set with storing the previous value as the tests currently do that is what we achieve, no? The scoped_test addition is meant to make features less painful to use and test.

The use of unsafe for features support isn't really the end of the world.

As noted in the PR description unsafe was not really needed, the API was just garbage, as you needed a &mut Features to modify features, but atomics can be modified with &self. The cleanup commit makes the whole Features-struct an implementation detail.

ridiculousfish · 2023-07-11T19:05:47Z

Merged, thank you!

krobelus · 2023-07-11T20:11:50Z

the API was just garbage

yeah this was ported literally from C++

faho · 2023-07-11T20:35:06Z

yeah this was ported literally from C++

And that's a sort of mismatch that is to be expected in a port like this. There is going to be a certain amount of friction between C++ and Rust, and a fairly literal port is the best way to make it tractable - divide and conquer, so you don't get lost in architectural adventures. The trick is in knowing when not to do that.

And as a side note: While I know that calling someone else's code "garbage" is common in other projects, I would prefer not to have that here.

henrikhorluck · 2023-07-11T20:40:52Z

And as a side note: While I know that calling someone else's code "garbage" is common in other projects, I would prefer not to have that here.

Poor choice of words from my side. "Inconsistent" would have been more appropriate

henrikhorluck force-pushed the safe-features branch 2 times, most recently from 74367c8 to ae861f0 Compare July 9, 2023 19:21

krobelus reviewed Jul 9, 2023

View reviewed changes

henrikhorluck force-pushed the safe-features branch from ae861f0 to 8e379ee Compare July 9, 2023 20:41

Clean up feature flags API

4d71af9

This also cleans up and removes unnecessary usage of FFI-oriented `feature_metadata_t`, which is only used from Rust code after `builtins/status` was ported.

henrikhorluck force-pushed the safe-features branch from 8e379ee to d9857f2 Compare July 9, 2023 20:45

henrikhorluck changed the title ~~Support safe feature flag setting and testing~~ Clean up Feature Flags API, add support for scoped feature tests Jul 9, 2023

henrikhorluck force-pushed the safe-features branch from f5bbec5 to 5a3f3bc Compare July 10, 2023 04:58

Disallow using set outside of tests, minor fixes

f8ab1e8

henrikhorluck force-pushed the safe-features branch from 5a3f3bc to 2a8a474 Compare July 10, 2023 05:06

Support thread-safe feature-flag-dependant tests

feeed4e

This also allows scoped feature tests that makes testing feature flags thread-safe. As in you can guarantee that the test actually has the correct feature flag value, regardless of which other tests are running in parallell.

henrikhorluck force-pushed the safe-features branch from 2a8a474 to feeed4e Compare July 10, 2023 05:14

henrikhorluck commented Jul 10, 2023

View reviewed changes

mqudsi reviewed Jul 10, 2023

View reviewed changes

ridiculousfish merged commit 63b2371 into fish-shell:master Jul 11, 2023

henrikhorluck deleted the safe-features branch July 11, 2023 20:42

zanchey added the rust label Dec 29, 2023

zanchey added this to the fish next-3.x milestone Dec 29, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Dec 30, 2024

Uh oh!

Uh oh!

Conversation

henrikhorluck commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krobelus Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

henrikhorluck Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

krobelus Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

henrikhorluck Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krobelus Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

henrikhorluck Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

ridiculousfish commented Jul 10, 2023

Uh oh!

henrikhorluck commented Jul 10, 2023

Uh oh!

henrikhorluck Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

mqudsi left a comment

Choose a reason for hiding this comment

Uh oh!

henrikhorluck commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ridiculousfish commented Jul 11, 2023

Uh oh!

krobelus commented Jul 11, 2023

Uh oh!

faho commented Jul 11, 2023

Uh oh!

henrikhorluck commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

henrikhorluck commented Jul 9, 2023 •

edited

Loading

henrikhorluck Jul 9, 2023 •

edited

Loading

henrikhorluck commented Jul 10, 2023 •

edited

Loading

henrikhorluck commented Jul 11, 2023 •

edited

Loading