Skip to content

Conversation

@tomtau
Copy link

@tomtau tomtau commented Mar 10, 2023

Description

  1. switch to rustls in tokio-tungstenite: we find rustls to be more portable, so we prefer using that. Maybe both native-tls and rustls can be made features of the client if someone prefers native-tls?
  2. switch ConnectionHandler to async_trait: we use tokio's sync primitives (async Mutex or channels), so with the old trait, one will need to e.g. call blocking_lock in the ConnectionHandler implementation... but that would panic, because the SDK's client will execute ConnectionHandler in an asynchronous context. So if ConnectionHandler was kept synchronous, it'd either require SDK to use spawn_blocking on ConnectionHandler's handler methods or the docs should say the implementor should make sure to use block_in_place in their methods... Anyway, the simplest solution seems to be to make ConnectionHandler asynchronous.

Resolves # N/A

How Has This Been Tested?

using cargo run --example basic_client --features full, plus a custom partial Sign API implementation

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@heilhead
Copy link
Collaborator

heilhead commented Mar 15, 2023

@tomtau Thanks for the PR!

Maybe both native-tls and rustls can be made features of the client if someone prefers native-tls?

Ideally, the client should be as flexible as possible, supporting both async-std and tokio, and provide some sane configuration options for its dependencies (e.g. rustls vs native-tls). But we're not there yet. So regarding the rustls support - can you please split it into a separate PR and make it a configurable feature of relay_client?

As for the ConnectionHandler being synchronous, the docs should indeed have a mention of this design decision. The reason it's synchronous is that it's not meant to be a final destination for message handling, and should never block, unless you specifically want to avoid processing inbound messages concurrently. ConnectionHandler is meant to act as an event router embedded into the socket event loop.

With that in mind, our initial idea was to use ConnectionHandler to pipe incoming messages into appropriate event channels (which is generally a non-blocking operation). And then have consumer code use those channels in whatever way they like. If you don't want to be bothered with an external event loop for message handling, it's also possible to tokio::spawn() a future from the handler, which is also a non-blocking operation and the messages will be processed concurrently:

impl ConnectionHandler for Handler {
    fn message_received(&mut self, message: PublishedMessage) {
        let name = self.name.clone();

        tokio::spawn(async move {
            println!(
                "[{}] inbound message: topic={} message={}",
                name, message.topic, message.message
            );
        });
    }

    // ...
}

And lastly, if none of the above suggestions fit your project needs, it's possible to build your own socket event loop using the ClientStream. See connection_event_loop() for an example.

@tomtau
Copy link
Author

tomtau commented Mar 16, 2023

Ok, thanks for the explanation.
I separated out the rustls feature here: #11

@tomtau tomtau closed this Mar 16, 2023
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