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

Integrate mixclient run fix #320

Merged

Conversation

danielSanchezQ
Copy link
Collaborator

No description provided.

@danielSanchezQ danielSanchezQ added this to the Mixnet milestone Aug 22, 2023
@danielSanchezQ danielSanchezQ self-assigned this Aug 22, 2023
@danielSanchezQ danielSanchezQ changed the base branch from master to integrate-mixclient-to-netbackend August 22, 2023 07:17
@danielSanchezQ danielSanchezQ marked this pull request as ready for review August 22, 2023 07:17
@danielSanchezQ danielSanchezQ force-pushed the integrate-mixclient-run-fix branch 2 times, most recently from d39e6ed to 0ca2e3a Compare August 22, 2023 09:04
@youngjoon-lee youngjoon-lee linked an issue Aug 22, 2023 that may be closed by this pull request
mixnet/client/src/receiver.rs Outdated Show resolved Hide resolved
Comment on lines 107 to 108
tracing::error!("Could not quickstart mixnet stream");
Box::pin(stream::empty())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if this need to be just an error or we should actually panic. If the service rely on it 🤔 . At least in the case where we actually return an error:

Ok(Some(stream)) => stream,
Ok(None) => log_error,
Err(e) => panic!

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, it's good to use stream::empty() so that the libp2p backend doesn't need to care about the mixclient mode and just read the stream that will never return something. Actually, I also think it would be okay to make the empty stream returned from the mixclient (instead of making the empty stream in the libp2p backend).

It's also reasonable to treat this as an error. Then, we need a check in the libp2p backend as below:

let stream = ...;   // maybe, empty() ?
if matches!(config.mixnet_client.mode, SenderReceiver(_)) {
  stream = mixnet_client.run();
}

...

loop {
  tokio::select! {
    Some(msg) = stream.next() -> {
      ..
    }
    Some(event) = swarm.next() -> {
      ..
    }
    ..
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, returning an empty stream from the client seems like a cleaner API option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I'm working on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

6a5e2e9 It took some time to struggle with the compiler :)

I adopted thiserror to avoid the following error when returning either stream::empty() or stream::unfold from mixclient, if the stream type is Stream<Result<_, Box<dyn Error>>>.

error[E0277]: `(dyn StdError + 'static)` cannot be sent between threads safely
  --> mixnet/client/src/config.rs:29:32
   |
29 |             Self::Sender => Ok(stream::empty().boxed()),
   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn StdError + 'static)` cannot be sent between threads safely
   |

I think it's good because we are going to use thiserror anyway at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You just need to add dyn Error + Send in the code. I think it is better to use thiserror. But I wouldn't like the PR to keep growing too much. We can do that as a refactor later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree. I tried to add Send but I faced another error, iirc. I agree with drawing the line here and merge this if it's okay.

Copy link
Collaborator Author

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

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

I think we can go ahead with this @youngjoon-lee

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

yeah, i think so, even though there are something to be improved. i will make issues for them.

@danielSanchezQ danielSanchezQ merged commit d54432b into integrate-mixclient-to-netbackend Aug 28, 2023
2 checks passed
@youngjoon-lee youngjoon-lee deleted the integrate-mixclient-run-fix branch September 1, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring MixnetClient::Receiver
2 participants