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

impl AsyncRead, AsyncWrite for native MixnetClient #4634

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

durch
Copy link
Contributor

@durch durch commented Jun 10, 2024

Heavily inspired by what @mfahampshire did for mix-tcp, folds AsyncRead functionality into the MixnetClient.

TODO:

  • Sink
  • AsyncWrite
  • Add examples

Interesting changes in wasm, due to Sink, we now need &mut for sending, so had to wrap stuff in RwLock in wasm client and mixfetch


This change is Reviewable

Copy link

vercel bot commented Jun 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
nym-explorer ⬜️ Ignored (Inspect) Visit Preview Jun 25, 2024 1:01pm
nym-next-explorer ⬜️ Ignored (Inspect) Visit Preview Jun 25, 2024 1:01pm

@durch
Copy link
Contributor Author

durch commented Jun 10, 2024

We can keep this one open for the other changes (AsyncWrite + Sink) as well, it will be easier to polish wholesale

@durch durch changed the title impl AsyncRead for native MixnetClient impl AsyncRead, AsyncWrite for native MixnetClient Jun 10, 2024
@durch durch marked this pull request as draft June 10, 2024 11:43
@jstuczyn
Copy link
Contributor

it's definitely not in the scope for this feature, but I wonder if conceptually it would make sense for AsyncRead and AsyncWrite to completely bypass message reconstructing and operate on raw bytes instead where it's up to the caller to deal with them as they see fit

@durch durch marked this pull request as ready for review June 14, 2024 08:21
@durch durch added this to the DoubleDecker milestone Jun 18, 2024
common/client-core/src/client/inbound_messages.rs Outdated Show resolved Hide resolved
sdk/rust/nym-sdk/src/mixnet/native_client.rs Outdated Show resolved Hide resolved
sdk/rust/nym-sdk/src/mixnet/native_client.rs Show resolved Hide resolved
@@ -48,10 +50,11 @@ pub(crate) trait InputSender {
fn send_messages(&self, messages: Vec<InputMessage>) -> Promise;
}

impl InputSender for Arc<ClientInput> {
impl InputSender for Arc<RwLock<ClientInput>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

for sanity check, have you actually tried to run the wasm code with those changes?
imo the simplest way, without dealing with bundling et al. would be to go to wasm/client, do make build-debug-dev and finally run the internal-dev project. though I'm not 100% sure how rotten it is at this point, so you might have to make tiny changes in worker.js

fn send_message(&self, message: InputMessage) -> Promise {
let this = Arc::clone(self);
future_to_promise(async move {
let mut this = this.write().await;
Copy link
Contributor

Choose a reason for hiding this comment

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

this limitation is a bit annoying, it means that whenever you want to send a message and waiting for the whole pipeline to go through, you can't receive anything : /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I didn't spend a lot of time on the WASM part, I'll revisit these. One could not await on the JS level, ie use a Promise::all or something like that

sdk/rust/nym-sdk/src/mixnet/native_client.rs Outdated Show resolved Hide resolved
sdk/rust/nym-sdk/src/mixnet/native_client.rs Outdated Show resolved Hide resolved
}
}

impl AsyncWrite for MixnetClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

imo AsyncWrite shouldn't be implemented on MixnetClient directly because conceptually it doesn't make a lot of sense. you're really just sending full messages as opposed to bytes. there should be some intermediate structure, like

struct MixnetClientAsyncWrite{
  recipient: Recipient
}

so then you'd have something like fn async_write(&self, recipient: Recipient) -> MixnetClientAsyncWrite on the client where you could just write your bytes your heart's content without having to do any encoding

Copy link
Contributor

Choose a reason for hiding this comment

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

now, for any "higher" level applications that might need AsyncRead + AsyncWrite maybe we could just make a wrapper like

struct Wrapper {
  inner: MixnetClient,
  // or whatever fields we need
  write_recipient: Recipient,
}

with maybe even Deref trait for easier use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a very good point, but I wanted to avoid adapters here, I'm gonna give it some more though, maybe with the new InputMessageCodec we can get to just sending bytes in, and they discretley get converted to messages...

sdk/rust/nym-sdk/src/mixnet/native_client.rs Show resolved Hide resolved
@benedettadavico benedettadavico modified the milestones: DoubleDecker, Wispa Jun 26, 2024
@durch durch modified the milestones: Wispa, TopDeck Jul 10, 2024
@benedettadavico benedettadavico modified the milestones: TopDeck, Caramello Jul 24, 2024
@durch durch marked this pull request as draft July 26, 2024 10:29
@benedettadavico benedettadavico modified the milestones: Caramello, Wedel Aug 5, 2024
@benedettadavico benedettadavico modified the milestones: Wedel, Aero Sep 11, 2024
@benedettadavico benedettadavico modified the milestones: Aero, Temporary Home Sep 20, 2024
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.

3 participants