[refactor] Switch from Rc<T> to Arc<T> #366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 56.84% 56.70% -0.14%
==========================================
Files 45 45
Lines 3339 3333 -6
Branches 832 832
==========================================
- Hits 1898 1890 -8
- Misses 884 886 +2
Partials 557 557
|
fabricedesre
force-pushed
the
arc-instead-of-rc
branch
from
6189076 to
ccdf22e
Compare
Yeah we're super not doing semver yet, but that's fine if that's another PR. Seems like the wasm code needs the same |
fabricedesre
force-pushed
the
arc-instead-of-rc
branch
from
ccdf22e to
35922c6
Compare
matheus23
approved these changes
Nov 7, 2023
matheus23
added a commit
that referenced
this pull request
Nov 28, 2023
The main goal of this PR is to enable using rs-wnfs in multithreaded contexts, e.g. in axum webservers. We have a test repo to check whether that works in an MVP: https://github.com/fabricedesre/wnfs-mtload/ Previously that wasn't possible, since the async futures from rs-wnfs weren't `Send`, so you couldn't have a multi-threaded work-stealing async runtime work on them. The reasons they weren't sync were: - Futures would capture an `impl BlockStore`, and it's not necessarily known to be `Send` - Futures would capture an `impl PrivateForest` with the same problem - Some functions would produce `LocalBoxFuture` or `LocalBoxStream`, which aren't `Send` - We'd use `async_trait(?Send)` and `async_recursion(?Send)` which opt-in to not having `Send` bounds, since that's what we need for wasm - Futures would capture internal WNFS data structures like `PrivateNode`, which would use `Rc` internally instead of `Arc`, see also #250 Some of this work was already addressed in #366. This PR *should* cover the rest. --- There's a complication with Wasm, where we're e.g. using an external type `extern "C" { type BlockStore; ... }`, which isn't `Send` or `Sync`, and as such can't ever implement a `trait BlockStore: Send + Sync`. To fix this, we're conditionally compiling in `Send` and `Sync` bounds (and `Arc` and `Rc` and similar) based on the target (See `send_sync_poly.rs`). This is pretty much just copying what noosphere is doing: https://github.com/subconsciousnetwork/noosphere/blob/main/rust/noosphere-common/src/sync.rs I'm hoping eventually we just fix this and thus enable multi-threaded Wasm, too. But for now this works. --- * wip - still need to fix the SnapshotBlockStore implementation * Fix SnapshotBlockStore impl * Fix wnfs=hamt * fix: `Send` bounds & `BytesToIpld` implementations Also: fix formatting * feat: Also make `PrivateForest` trait `Send + Sync` Also: Remove unneeded `Sync` bounds left over from previous commits. * feat: Use `BoxFuture` instead of `LocalBoxFuture` in `PrivateForest` fn * feat: Remove `(?Send)` annotations everywhere * feat: Conditionally compile `Send + Sync` bounds This relaxes the requirement if you're not on the `wasm32` target. The problem is that foreign types in Wasm don't implement `Sync`. * chore: Fix all tests & doctests to use thread-safe RNGs --------- Co-authored-by: Fabrice Desré <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes/implements the following bugs/features
RctoArc#250Closing issues
closes #250
After Merge
I didn't bump the crate version number yet. This change is breaking the API backward compatibility so that should be a major semver change, but I'll let you decide what you want to do about it.