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

Multithreaded Client #2321

Merged
merged 8 commits into from Feb 6, 2023
Merged

Multithreaded Client #2321

merged 8 commits into from Feb 6, 2023

Conversation

cavemanloverboy
Copy link
Contributor

Addresses #2218.

This introduces two changes.

  1. Internally, it has the client library hold and accept some S: Signer + Send + Sync instead of some dyn Signer. This makes the Client (and other) types Send and Sync.
  2. We change all std::rc::Rc to std::sync::Arc so that the Client (and other) types can be shared across threads. This allows builders to write multithreaded (async, parallel) codes that use an anchor_client::Client.

We also update the client/example to include a single and multithreaded test..

@vercel
Copy link

vercel bot commented Dec 18, 2022

@cavemanloverboy is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 18, 2022

This seems cool for sure but looks like a major breaking change / a lot of rewriting will be needed by anyone using the new client? Just because it looks like almost all the functions / a bunch of types need <S> after them now. Plus I'm totally dumb and was wondering what even is S in this context. (Reading into it more it looks like S is the keypair / signer type but still haven't cleared up how it works.)

There must be a more ergonomic way to support multi thread than having to add <S: Signer + Send + Sync> in front of all functions now fn basic_2<S: Signer + Send + Sync>(client: &Client<S>, pid: Pubkey) -> Result<()> {. Or is this syntax just for functions that will support multithreading?

@Henry-E
Copy link
Contributor

Henry-E commented Dec 21, 2022

I think this is good to merge just two things

  • please add it as a breaking change to the changelog
  • Based on some CI test errors, it looks like the zero copy test uses the client and is broken, so it also needs to be updated

Hopefully we can merge and someone of the insane people who use anchor/master to dev off of can give comments on how it affects their client experience!

@Henry-E Henry-E added the blocked label Jan 4, 2023
@cavemanloverboy
Copy link
Contributor Author

BTW this should no longer introduce breaking changes and all tests pass

@Henry-E
Copy link
Contributor

Henry-E commented Feb 3, 2023 via email

@Henry-E
Copy link
Contributor

Henry-E commented Feb 6, 2023

I'm going to merge this before it goes on any longer but before releasing the next version of anchor it would be really helpful if you were able to write up something about how to use the new multithreaded option and whether anything needs to be changes about existing tests to use it. e.g. In the example test there's a slightly complicated type alias for the test function, should all multithreaded tests be written using such a pattern / type?

@Henry-E Henry-E merged commit 020b644 into coral-xyz:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants