ALT DRAFT RFC: add option to use alloc::rc::Rc for no-std targets w/o built-in atomic ops - INITIAL HACK #2088
Conversation
…mics built-in with the following new features: * withrcalias - use an internal alias to use alloc::rc::Rc instead of alloc::sync::Arc removes Send + Sync from many pub traits, making it impossible to share config state between threads * defaultproviderenabled - use this feature to enable default provider API functions will not work together with the withrcalias feature APPROACH: * add & use new, internal alias module to alias alloc::rc::Rc as Arc for withrcalias feature * add & use new, internal macros to define certain traits with or without Send + Sync, depending on whether withrcalias feature is absent or present
rustls/Cargo.toml
Outdated
| # XXX TODO ADD THIS TO DEFAULT FEATURES | ||
| defaultproviderenabled = [] | ||
| # XXX TODO NAMING - ADD SEPARATORS | ||
| withrcalias = [] |
There was a problem hiding this comment.
I will likely propose a different solution for this, as discussed elsewhere thanks.
| #[cfg(not(feature = "withrcalias"))] | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
Should merge double cfg attributes everywhere.
Why disable this for withrcalias?
There was a problem hiding this comment.
I recall using Rc led to an issue with this due to lack of Send + Sync marker traits.
| #[cfg(not(feature = "withrcalias"))] | ||
| macro_rules! pub_api_trait { | ||
| ($name:ident, $body:tt) => { | ||
| pub trait $name: core::fmt::Debug + Send + Sync $body |
There was a problem hiding this comment.
I haven't tried this, but I was assuming this would work:
#[cfg(target_has_atomic = "ptr")]
pub trait ThreadSafe : Send + Sync {}
#[cfg(not(target_has_atomic = "ptr"))]
pub trait ThreadSafe {}(then each extension point trait would be pub trait ResolvesServerCert: Debug + ThreadSafe { ... })
There was a problem hiding this comment.
I did already try this kind of idea, discovered that API user would need to add additional empty impl block for the added "ThreadSafe" trait. Looks like Rust unstable would support trait alias, not sure how well this would work for conditional usage of the Send + Sync marker traits.
rustls/Cargo.toml
Outdated
| # XXX TODO ADD THIS TO DEFAULT FEATURES | ||
| defaultproviderenabled = [] | ||
| # XXX TODO NAMING - ADD SEPARATORS | ||
| withrcalias = [] |
There was a problem hiding this comment.
Can we actually do this with features? It causes a public API breaking change. Perhaps we can use #[cfg(target_has_atomic = "ptr")], which simply echoes the non-portability of Arc?
There was a problem hiding this comment.
I would totally like to get rid of the new features, do have a couple questions:
Any pointers or ideas how we might test this in CI (and on a local workstation)?
I would also love to add an option to keep the Arc support working with the portable-atomics crate. This could increase the complexity of the cfg conditions in multiple places. We could try to address this at some point in the future, with a little extra code churn.
…x crypto::install_default() - consistent API std vs non-std ... XXX TODO SPLIT THIS UPDATE OUT TO SEPARATE PR
rustls/src/crypto/mod.rs
Outdated
| #[cfg(feature = "defaultproviderenabled")] | ||
| mod crypto_default_provider { | ||
| use alloc::sync::Arc; |
There was a problem hiding this comment.
I have already proposed this update in PR #2089. I am planning to update this cfg attribute to get rid of the new feature options.
…i_trait_with_doc! & remove old pub_api_trait! macro
…t::{ClientSessionStore, ResolvesClientCert}"
This reverts commit c08bcb1.
…SSING SUPPORT for alloc::sync on thumbv6m
… is good practice for a library) ... with mutliple caveats to help avoid excess complexity in rustls/src/crypto/mod.rs: - explicit critical-section now REQUIRED for non-std - enforced by compile error for now - critical-section is now ALWAYS ENABLED in case withrcalias is enabled add build & run steps with explicit critical-section remove some commented code now obsolete from rustls/src/crypto/mod.rs XXX TODO: SHOULD SUPPORT no-std without critical-section required (see portable-atomic crate for description why critical-section should not be required in a library)
…e_cell/tree/propagate-critical-section-to-portable-atomic - no more need for explicit portable-atomic dependency with explicit "portable-atomic/critical-section" setting for critical-section option
| /// This will be `None` if no default has been set yet. | ||
| #[cfg(not(feature = "withrcalias"))] | ||
| pub fn get_default() -> Option<&'static Arc<Self>> { | ||
| static_default::get_default() | ||
| } | ||
|
|
||
| /// Returns the default `CryptoProvider` for this process. | ||
| /// | ||
| /// This will be `None` if no default has been set yet. | ||
| pub fn get_default() -> Option<&'static Arc<Self>> { | ||
| PROCESS_DEFAULT_PROVIDER.get() | ||
| #[cfg(feature = "withrcalias")] | ||
| pub fn get_default() -> Option<Arc<Self>> { | ||
| static_default::get_default() | ||
| } |
There was a problem hiding this comment.
Unfortunately this API function has a somewhat different return type depending on the build configuration.
Existing return type with static-lifetime reference to Arc cannot be replicated with Rc alias due to differences in storing & cloning.
I would rather not break existing API usage for any existing users.
I think I need to add a note to the source to explain this.
…n rustls/Cargo.toml
with help from sed:
% sed -i.orig~~ "s/#.*NOTE:/# NOTE:/g" rustls/Cargo.toml
verified that the following now shows no more weird results:
% git grep "#.*NOTE:" | grep -v "# NOTE:"
…omment in rustls/Cargo.toml
|
Just wanted to leave a note here that I'm going to wait to review this until it's out of draft status and the commit history is easier to approach. Please tag if there are specific questions I could try to answer in the meantime! |
…te XXX comment in rustls/Cargo.toml" This reverts commit 68639d3.
|
@cpu it looks to me like updating I am now thinking of taking another approach which I may end up proposing in a brand-new PR. My apologies for all of the noise with this one! |
Going to flip this back to draft for now, then. |
|
I would like to close this one for now. I think that the alternative I proposed in PR #2200 should be much cleaner & easier to understand. I do think this would be an interesting option to consider adding in the future, but it would likely be less drastic if we can finish & merge #2200 first. P.S. I think & hope that I have addressed all feedback from here that may apply to PR #2200. |
STATUS: initial draft with some ugly hacks & ugly naming - needs some cleanup & discussion before merging - can we find a way to break this update into smaller & cleaner pieces?
WHAT
implement idea 1 discussed in: #2068 /cc @bjoernQ
with the following new feature
s(need to discuss & improve naming):withrcalias- use an internal alias to usealloc::rc::Rcinstead ofalloc::sync::Arc; this removesSend + Syncfrom many pub traits, making it impossible to share config state between threadsdefaultproviderenabled- use this feature to enable default provider API functions, which will not work together with thewithrcaliasfeature - TODO item is to add this to the default feature list (I encountered some issues with this, think I encountered this: cargo test --all don't honor --no-default-features for member project rust-lang/cargo#7160 (comment))I decided to keep these as separate options as well, may make sense to consider combining them in some form / fashion.May include some other, very minor code & testing updates, for example:
withrcaliasfeature enableduse_default_with_exactly_one_providerfor improved clarityAPPROACH
aliasmodule to aliasalloc::rc::RcasArcwhen thewithrcaliasfeature is enabledpub_api_trait&internal_generic_state_traitmacros to define certain traits with or withoutSend + Sync, as needed for the build to succeed, depending on whetherwithrcaliasfeature is absent or presentusedefaultproviderenabledfeature configuration to enable or disable default provider API functions, which will not build or work when thewithrcaliasfeature is enabledcrypto_default_providermodule to manage default crypto provider state in a cleaner manner, regardless ofwithrcaliasfeature, with thanks toonce_cellwithcritical-sectionfeatureNOTE that this includes an update I proposed in #2087, as needed to build when thewithrcaliasfeature is enabled. (PR #2087 is now merged & merged into this proposal)RATIONALE
alloc::rc::Rcinstead ofalloc::sync::Arcenables use with no-std targets w/o built-in atomic ops, as described inno-stdsupport for targets w/o atomics? #2068, without the CI issues described inno-stdsupport for targets w/o atomics? #2068 (comment)defaultproviderenabledfeature enables the build to succeed withSend + Syncremoved from many pub API traitsHOW TESTED
cargo build -p rustls -F withrcalias(try building of package withwithrcaliasfeature enabled)cargo build -F defaultproviderenabled(try full build withdefaultproviderenabledfeature enabled)cargo test -p rustls -F withrcalias(unit test of package withwithrcaliasfeature enabled)cargo test -F defaultproviderenabled(full unit testing withdefaultproviderenabledfeature enabled)cargo test -p rustls(less important - unit test of package with both new feature options not enabled)thumbv6m-none-eabi, with no atomic pointer functionalityTODO items & loose ends
git grep "pub.*trait.*Send"shows some more public traits that should probably be updated to usepub_api_traitmacro, to removeSend + Syncwhen thewithrcaliasfeature is enabled - it may be ideal to add some more unit tests to check thatSend + Syncis gone for these public traits with optionalwithrcaliasfeature--all-featurescargo option (I am wondering ifcustom-providercould lead to a similar issue with built-in providers not enabled)git grep XXXshould not show any new results with this proposal#[cfg(...)]formatting