Expose `into_service` functions that provide Sync and/or Clone Services

banool commented 1 year ago

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 (https://github.com/tower-rs/tower/pull/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: https://github.com/tower-rs/tower/issues/691. See also https://github.com/tower-rs/tower/issues/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 commented 1 year ago

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 commented 1 year ago

Okay I can continue this investigation with Poem :)

banool commented 1 year ago

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 commented 1 year ago

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)

hyperium / tonic