Consider making some sv1_api::IsServer trait methods into async

[plebhash]

an adopter asked why the Sv1 handlers are syncrhonous

more specifically, they reported that they are building an authentication flow that would be better if sv1_api::IsServer::handle_authorize were async

[Fi3]

cause otherwise the library would be very very impractical to use without an async runtime. The idea is that you don't block in the handler. Having them not async allow the handlers to be called from an async context and and from outside an async context.

for me concept nack

[AkulovOleg]

cause otherwise the library would be very very impractical to use without an async runtime. The idea is that you don't block in the handler. Having them not async allow the handlers to be called from an async context and and from outside an async context.

for me concept nack

Hi, @ Fi3 I believe all code should be async, especially authentication flow. For example, we need to implement an HTTP request in this handler to get info miner is authenticated or no. If we implement it synchronously it will not work with high load as there will be thread blocking.

[mlgodskz]

cause otherwise the library would be very very impractical to use without an async runtime. The idea is that you don't block in the handler. Having them not async allow the handlers to be called from an async context and and from outside an async context.

Does this mean approximately this, or something else?

fn handle_authorize(&self, request: &client_to_server::Authorize) -> bool {

        let worker_name = request.name.to_string();

        tokio::task::block_in_place(|| {
            tokio::runtime::Handle::current().block_on(async {
...
let result = client
                    .post(&api_url)
                    .header("X-Api-Key", api_key)
                    .header("Content-Type", "application/json")
                    .json(&serde_json::json!({
....
                    }))
                    .send()
                    .await;
...

[jbesraa]

yea block_in_place or https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html can be used to run sync in async context

[mlgodskz]

cause otherwise the library would be very very impractical to use without an async runtime. The idea is that you don't block in the handler. Having them not async allow the handlers to be called from an async context and and from outside an async context.

Can your words be interpreted as meaning there are no significant architectural obstacles to making the fn handle_authorize asynchronous? I'm referring to any potential pitfalls not immediately apparent from a superficial code review, but which could make such refactoring quite labor-intensive.

[AkulovOleg]

yea block_in_place or https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html can be used to run sync in async context

It will not work properly when you connect 10k workers immediately

[plebhash]

cause otherwise the library would be very very impractical to use without an async runtime. The idea is that you don't block in the handler. Having them not async allow the handlers to be called from an async context and and from outside an async context.

Can your words be interpreted as meaning there are no significant architectural obstacles to making the fn handle_authorize asynchronous? I'm referring to any potential pitfalls not immediately apparent from a superficial code review, but which could make such refactoring quite labor-intensive.

I would not interpret it that way.

Please keep in mind that all SRI low-level libs (under protocols directory) are written with the following architectural pre-requisites:

• embedded-friendly
• least possible external dependencies

by forcing them to be async, we introduce a new pre-requisite of having some async runtime available for every possible use-case, which is not easy to reconcile with the two pre-requisites listed above

overall I tend to agree with @Fi3 that it doesn't seem to make sense to impose async for every possible use-case of this lib

and if implemented correctly, async use cases are still perfectly doable with sv1_api as is

[plebhash]

yea block_in_place or https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html can be used to run sync in async context

It will not work properly when you connect 10k workers immediately

isn't this something that could be handled at infrastructure level?

[mlgodskz]

or https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html can be used to run sync in async context

Yes, that would be good, but fn handle_authorize is already called within the Tokio runtime, and any attempts to make it asynchronous result in an error. Using block_in_place, as Oleg mentioned, severely limits the system's ability to handle many concurrent connections (over 1,000). Oddly enough, the most stable result currently comes from using std::thread::spawn within fn handle_authorize to interact with the external authorization server. However, this seems to consume significantly more resources than an asynchronous approach would. In summary, for the time being, it still looks like we need to make fn handle_authorize and its related functions asynchronous.

[jbesraa]

Why is it a problem that handle_authroize is called within a tokio rt?
If you are able to share some code we might be more helpful.

[plebhash]

as @GitGab19 pointed out in a chat, one potential middle ground could be to expand sv1_api to add async support to it as an extra feature

perhaps a new trait called IsServerAsync providing the desired alternatives

this way, we wouldn't force every consumer of the crate to use an async runtime, they could still use IsServer and only resort to IsServerAsync for specific use-cases

[plebhash]

as @GitGab19 pointed out in a chat, one potential middle ground could be to expand sv1_api to add async support to it as an extra feature

perhaps a new trait called IsServerAsync providing the desired alternatives

this way, we wouldn't force every consumer of the crate to use an async runtime, they could still use IsServer and only resort to IsServerAsync for specific use-cases

I did a quick experiment which consisted of:

• add a new IsServerAsync trait to src/lib.rs of sv1_api crate
• add a async fn handle_message function to this trait, while keeping the entire API and function body identical to IsServer (only async keyword was added)

the compiler built the code, but gave the following warning:

warning: use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
   --> v1/src/lib.rs:236:5
    |
236 |     async fn handle_message(
    |     ^^^^^
    |
    = note: you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
    = note: `#[warn(async_fn_in_trait)]` on by default

after googling for the waning message, the following resources come up:

• https://users.rust-lang.org/t/async-in-public-trait/108400
• https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html

[plebhash]

I made some extra progress on the experiment described above.

I continued with the strategy of adding a new IsServerAsync trait to sv1_api crate. After reading the blogpost on the official Rust blog, I realized that I could make the warning go away by simply adding a #[async_trait] macro (provided by async-trait) crate. You can check that on this commit plebhash@c2ae096

I kindly ask @mlgodskz and @AkulovOleg to do some validation. Please change your Cargo.toml so that your sv1_api dependency comes from the sv1_api_async branch of my fork.

[mlgodskz]

Why is it a problem that handle_authroize is called within a tokio rt?
If you are able to share some code we might be more helpful.

Currently, we're interacting with the external authorization server like this:

fn handle_authorize(&self, request: &client_to_server::Authorize) -> bool {
    
        let handle = std::thread::spawn(move || {

            ...
    
            let client = reqwest::blocking::Client::builder()
                .timeout(std::time::Duration::from_secs(timeout_seconds))
                .build()
                .unwrap();
    
            let result = client
                .post(&api_url)
                .header("X-Api-Key", api_key)
                .header("Content-Type", "application/json")
                .json(&serde_json::json!({
                    ...
                }))
                .send();
    
            match result {
                Ok(response) => {
                    if let Ok(json) = response.json::<serde_json::Value>() {
                        ...
                    }
                    false
                }
                Err(e) => {
                    false
                }
            }
        });   
        handle.join().unwrap_or(false)
    }

All other options either don't work because this function is already called within the Tokio runtime, or perform worse, such as using block_in_place(). Besides refactoring to async/await, what other optimizations could be applied here?

[Fi3]

@plebhash adding the async trait or an async method under a flage seems a good option. @mlgodskz In the meantime you can do something like the below, assuming that you have an IsServer for each dowstream, otherwise you can use almost the same logic (you hold on messages from a specific downstream not all messages).

fn handle_authorize(&self, request: &client_to_server::Authorize) -> bool {
   self.on_hold = true; // you put this dowstream on hold no processing new messages
   self.authorize.send(request); // you send the message to a task that handle you authorization logic^1
   false
}

^1 task will need to have Arc<Mutex<Self>> (where self is your IsServer) or an some kind of channel so that when authorized you can do self.authorize(..) for example you can have an oneshoot recv that try recv when your downstream is on hold and return authorized or not authorized if you don't want to use mutexes

[jbesraa]

Why is it a problem that handle_authroize is called within a tokio rt?
If you are able to share some code we might be more helpful.

Currently, we're interacting with the external authorization server like this:

fn handle_authorize(&self, request: &client_to_server::Authorize) -> bool {
    
        let handle = std::thread::spawn(move || {

            ...
    
            let client = reqwest::blocking::Client::builder()
                .timeout(std::time::Duration::from_secs(timeout_seconds))
                .build()
                .unwrap();
    
            let result = client
                .post(&api_url)
                .header("X-Api-Key", api_key)
                .header("Content-Type", "application/json")
                .json(&serde_json::json!({
                    ...
                }))
                .send();
    
            match result {
                Ok(response) => {
                    if let Ok(json) = response.json::<serde_json::Value>() {
                        ...
                    }
                    false
                }
                Err(e) => {
                    false
                }
            }
        });   
        handle.join().unwrap_or(false)
    }

All other options either don't work because this function is already called within the Tokio runtime, or perform worse, such as using block_in_place(). Besides refactoring to async/await, what other optimizations could be applied here?

If this is running in a tokio runtime, I would let tokio handle the threading. So using tokio::task should be preferable than std::thread.

Maybe running tokio::task and using reqwest async API instead of blocking will improve performance.