Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Make rs-wnfs work in multithreaded contexts #372

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Conversation

matheus23
Copy link
Member

@matheus23 matheus23 commented Nov 21, 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 Refactor: Consider switching from Rc to Arc #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.

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #372 (cfe2ed9) into main (30ed01b) will increase coverage by 0.19%.
The diff coverage is 57.74%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   56.70%   56.89%   +0.19%     
==========================================
  Files          45       45              
  Lines        3333     3334       +1     
  Branches      832      830       -2     
==========================================
+ Hits         1890     1897       +7     
+ Misses        886      884       -2     
+ Partials      557      553       -4     
Files Coverage Δ
wnfs-common/src/async_serialize.rs 33.33% <ø> (ø)
wnfs-common/src/encoding.rs 62.50% <ø> (ø)
wnfs-common/src/link.rs 19.76% <ø> (ø)
wnfs-common/src/utils/common.rs 60.00% <ø> (ø)
wnfs-hamt/src/hamt.rs 41.02% <ø> (ø)
wnfs-hamt/src/pointer.rs 21.87% <ø> (ø)
wnfs/src/lib.rs 90.00% <ø> (ø)
wnfs/src/private/file.rs 73.68% <ø> (ø)
wnfs/src/private/forest/traits.rs 100.00% <ø> (ø)
wnfs/src/private/keys/exchange.rs 69.56% <ø> (ø)
... and 24 more

Also: Remove unneeded `Sync` bounds left over from previous commits.
@matheus23 matheus23 marked this pull request as ready for review November 21, 2023 19:42
@matheus23 matheus23 requested a review from a team as a code owner November 21, 2023 19:42
@matheus23
Copy link
Member Author

Replaces #368

@matheus23
Copy link
Member Author

matheus23 commented Nov 21, 2023

TODO:

  • Look at remaining async_trait(?Send):
    • #[async_trait(?Send)] pub trait ExchangeKey
    • #[async_trait(?Send)] pub trait PrivateKey
    • WnfsBlockStore and similar wrappers in Wasm code
  • Fix wnfs-wasm crate type errors.

@matheus23 matheus23 self-assigned this Nov 21, 2023
@matheus23 matheus23 changed the title Make BlockStore Send and Sync feat: Make rs-wnfs work in multithreaded contexts Nov 28, 2023
Copy link
Collaborator

@fabricedesre fabricedesre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great! Do you plan to do a version bump and release in another PR?

@matheus23
Copy link
Member Author

That looks great! Do you plan to do a version bump and release in another PR?

Yep. Soon-ish ✌️

@matheus23 matheus23 merged commit 98d43cb into main Nov 28, 2023
10 checks passed
@matheus23 matheus23 deleted the blockstore-send branch November 28, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants