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

Expose into_service functions that provide Sync and/or Clone Services #1322

Open
banool opened this issue Mar 19, 2023 · 4 comments
Open

Expose into_service functions that provide Sync and/or Clone Services #1322

banool opened this issue Mar 19, 2023 · 4 comments

Comments

@banool
Copy link

banool commented Mar 19, 2023

Feature Request

Motivation

I have the following code:

use anyhow::{Context as AnyhowContext, Result};
use aptos_api::context::Context;
use aptos_protos::api::v2::{
    api_v2_server::{ApiV2, ApiV2Server},
    GetAccountModuleRequest, GetAccountModuleResponse,
};
use poem::{endpoint::TowerCompatExt, IntoEndpoint, Route};
use std::sync::Arc;
use tonic::{transport::Server, Request, Response, Status};
use sync_wrapper::SyncWrapper;

#[derive(Clone)]
pub struct ApiV2Service {
    pub context: Arc<Context>,
}

pub fn build_api_v2_service(context: Arc<Context>) -> Result<impl IntoEndpoint> {
    let service = ApiV2Service { context };

    let tower_service = Server::builder()
        .add_service(ApiV2Server::new(service))
        .into_service();

    let tower_service = SyncWrapper::new(tower_service).compat();

    // https://github.com/poem-web/poem/issues/536
    let routes = Route::new().nest("/", tower_service);
    Ok(routes)
}

#[tonic::async_trait]
impl ApiV2 for ApiV2Service {
    async fn get_account_module(
        &self,
        request: Request<GetAccountModuleRequest>,
    ) -> Result<Response<GetAccountModuleResponse>, Status> {
        unimplemented!();
    }
}

In this code you see that I have a Tonic service which I convert into a Tower service. I then run it through this compat function from Poem so I can use it with Poem. The problem is that compat function (https://docs.rs/poem/latest/poem/endpoint/trait.TowerCompatExt.html) requires that the service be Sync, but tonic services are not Sync. Diving deeper, it is not as simple as making the Service Sync in tonic, that doesn't work. However when I look at Tower, I see that they have this BoxService which is Sync (tower-rs/tower#702). But with that I'm also having a variety of problems, because that isn't Clone (another requirement).

In any case, getting a Service that is either Clone or Sync is possible using these various utilities from Tower. IT seems like getting one that is both Clone and Sync is also possible, but TBA on that: tower-rs/tower#691. See also tower-rs/tower#663.

Proposal

So the request here is for Tonic to provide functions that give you these different Services out of the box. Figuring all this out took quite a while, it'd be nice if future Tonic users didn't have to worry about this and could just use functions like this:

  • into_service: Already exists.
  • into_service_sync: Like into_service but the service is Sync.
  • into_service_clone: Like into_service but the service is Clone.
  • into_service_sync_clone: Like into_service but the service is Sync + Clone.

Under the hood Tonic would utilize all these Tower utilities for us.

Alternatives

Users can do it all on their own after getting the service from into_service, but it is quite complicated. It'd be nice if this was just possible out of the box.

@LucioFranco
Copy link
Member

Oh this sounded like a fun one to explore :)

I am not sure if we want to provide all those methods. I think the issue in the first place is that poem requires sync when you shouldn't need it. Tonic originally required sync for all services but that was deemed a mistake. I would maybe reach out there and see if they would allow you to remove that restriction?

@banool
Copy link
Author

banool commented Mar 20, 2023

Okay I can continue this investigation with Poem :)

@banool
Copy link
Author

banool commented Mar 29, 2023

Unfortunately so far no cigar on the Poem side, so this problem remains. Can you think of any other way to get these two webserver frameworks to work together without having to run them both separately and put a reverse proxy in front?

@LucioFranco
Copy link
Member

Ok one suggestion would be to avoid using the tonic server all together just use the service directly. I realize the multiplex example uses Server but this one does not https://github.com/hyperium/tonic/blob/master/examples/src/hyper_warp/server.rs#L64.

If you need a router, I would implement that manually. Each generated *Server type contains a NamedService trait that has the routing info. This way you don't need to worry about the send/sync I believe it should just work. (I hope)

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

No branches or pull requests

2 participants