unstable feature to support targets with no atomic pointers - REJECTED DRAFT PROPOSAL #2200
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2200 +/- ##
==========================================
- Coverage 94.82% 94.82% -0.01%
==========================================
Files 104 104
Lines 24100 24172 +72
==========================================
+ Hits 22853 22920 +67
- Misses 1247 1252 +5 ☔ View full report in Codecov by Sentry. |
8f4a748 to
cb94a72
Compare
b22e503 to
aaad952
Compare
|
I think it's unlikely we will want to merge this in something close to its current form. rustls is a small project with a small group of maintainers and we don't necessarily have in-house expertise on embedded platforms and their needs. We also have API compatibility constraints that we want to hold up for the benefit of the (much larger) ecosystem of folks deploying rustls to more general purpose platforms (that is, where I haven't reviewed this in detail since I don't think it's close to being reviewable, in the sense of having (a) a high-level description of the constraints that made rustls a tricky fit for your platform and how you overcame these challenges and (b) a clean commit history with commits that incrementally change rustls to fit your needs. Instead of this approach, I think a better solution might be to incrementally modularize rustls more such that you can build on top of the logic that rustls provides without the rustls project having to maintain a bunch of workarounds for the limits of your platform (as in, could we provide an API that doesn't rely on |
|
Yes, I agree. I'm also pretty concerned about incorporating types from another crate in our public API (even behind a feature gate); I feel that would make cutting a rustls 1.0 even harder. I would perhaps be willing to entertain something like: diff --git a/rustls/src/lib.rs b/rustls/src/lib.rs
index d8562932..dc428236 100644
--- a/rustls/src/lib.rs
+++ b/rustls/src/lib.rs
@@ -674,6 +674,11 @@ pub mod lock;
/// Polyfills for features that are not yet stabilized or available with current MSRV.
pub(crate) mod polyfill;
+mod sync {
+ // This crate refers to Arc only via this point.
+ pub(crate) use alloc::sync::Arc;
+}
+
#[cfg(any(feature = "std", feature = "hashbrown"))]
mod hash_map {
#[cfg(feature = "std")]That would make such a fork much easier to maintain, as the difference would be limited to one place (assuming the replacement for Arc was truly interchangeable.) |
|
Any pointers how we could start improving the structure of this project? From a quick look through this project I did already start having some ideas on reducing the amount of dependency on I personally may not mind that |
|
Long-term seeing Rustls being more modular (and offering Arc-free APIs) would be a good thing. I was also hoping for not going the fork-route but if that fork can be easily maintained that's better than nothing |
In my mind we mostly use However, this is just spitballing and we'd have to carefully control how to allocate the extra complexity, so whoever takes this on needs to walk a fine line. |
If I would continue with this kind of idea, to help with the possible forking, should I continue in this PR or raise a brand new PR to do this?
After some trial-and-error in my personal workspace, I am now thinking of an idea to use config builders to build immutable config objects, that could be used from multiple threads, hopefully without all of the atomic locking requirements that come with P.S. I am now working on PR #2285 to propose over-writable While I do think it would be ideal to offer an option to use |
…-section feature, with MULTIPLE XXX TODO ITEMS
… minor effects in Cargo.lock)
…ml (with XXX / TODO ITEM)
…NT for this feature
…, if enabled by `critical-section` feature (with XXX / TODO ITEMS)
…cal-section feature is enabled includes exported util mod to export Arc alias, as helper for tests, rustls users, and custom rustls providers
… feature (WITH MANY XXX / TODO ITEMS)
f0f1d7c to
eb923b1
Compare
|
I think #2285 is our preferred way to support targets w/o atomic pointers. Let's close this PR for now since it has conflicts with main and doesn't seem likely to be merge-able in the near future. Thanks! |
STATUS: see TODO items below
Proposal to resolve issue: #2068
This is a proposal to support targets w/o atomic ptr by using portable-atomic
Arc&critical-section, as optional features, with help from a recently published contribution: taiki-e/portable-atomic#195This solution requires use of Rust nightly in order for the required coercion to work with the recent update from taiki-e/portable-atomic#195 ... looks like the stabilization is blocked pending this: rust-lang/rust#18598
Optional features added to
rustls/Cargo.toml:portable-atomic-arc- enable use ofArcfrom portable-atomic-utilcritical-section- enablecritical-sectionfeature on once-cell & portable-atomic-util crates & includeportable-atomic-arcfeature - this is needed to support build on targets w/o atomic ptrCI TESTING:
cross buildwithcritical-sectionon multiple targets including non-atomic ptr targetCOMPARISON TO ALTERNATIVE SOLUTIONS:
Rcfor target(s) with no atomic ptr - already proposed in PR ALT DRAFT RFC: add option to use alloc::rc::Rc for no-std targets w/o built-in atomic ops - INITIAL HACK #2088 - seems to imply some API mangling & use of an ugly macro to enable or disable use ofSend&Synctraits; possibly harder to document & understand the updated APISTATEMENT OF IMPACT
This unstable
critical-sectionfeature would require using Rust nightly along with the unstable 8000--cfg portable_atomic_unstable_coerce_unsizedbuild option in order for the build to succeed with this feature.The one form of impact I can think of is on users building rustls library with
--all-featuresflag. I do NOT think this should affect anyone using rustls as a dependency for a higher-level library or application.Alternative could be to use an unstable cfg option, like I had to do in taiki-e/portable-atomic#195, but this seems to be inconsistent with the other rustls build options.
XXX / TODO ITEMS:
.github/workflows/build.ymlrustls/src(rustls/src/lib.rs&rustls/src/crypto/mod.rs)sync::Arcalias to support fork for targets with no atomic ptr #2285)/cc @bjoernQ @taiki-e