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

Simplified Handle trait #1275

Closed
wants to merge 1 commit into from
Closed

Simplified Handle trait #1275

wants to merge 1 commit into from

Conversation

cetra3
Copy link
Contributor

@cetra3 cetra3 commented Jan 14, 2020

As discussed in the chat, there is no easy migration path for handlers from 0.7. The only way you can respond to requests is free floating functions. You should be able to impl a trait on structs to easily encapsulate data and use this accordingly.

This PR, while it probably won't be accepted initially, is my take on what I would like to see to simplify this process.

It defines a much simpler trait than the existing Service trait which requires 2 trait impls and a number of associated types.

The trait is as follows (Could possibly be generic on Error):

pub trait Handle {
    type Future: Future<Output= Result<ServiceResponse, Error>>;

    fn call(&mut self, req: ServiceRequest) -> Self::Future;
}

Then a struct (as we can't use specialisation yet) which requires that your struct must be clone so that service factory can be implemented. There might be other ways to accomplish this.

Example of usage:

#[derive(Clone)]
struct TestService;

impl Handle for TestService {
    type Future = Ready<Result<ServiceResponse, Error>>;

    fn call(&mut self, req: ServiceRequest) -> Self::Future {
        ok(req.into_response(HttpResponse::Ok().body("Test")))
    }
}
async fn main() -> std::io::Result<()> {
    std::env::set_var("RUST_LOG", "actix_web=debug");
    env_logger::init();

    HttpServer::new(|| {
        App::new()
            .service(web::service("/").handler(TestService)))
    })
    .bind("127.0.0.1:8080")?
    .run()
    .await
}

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #1275 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
- Coverage   80.47%   80.43%   -0.04%     
==========================================
  Files         159      159              
  Lines       18541    18550       +9     
==========================================
  Hits        14921    14921              
- Misses       3620     3629       +9
Impacted Files Coverage Δ
src/service.rs 80.84% <0%> (-3.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bca41f8...20cc54a. Read the comment docs.

@fafhrd91
Copy link
Member

Your Handle trait is just specific version of Service trait. But it misses two features
Async initialization, which is ServiceFactory does. and backpressure support, Service implements backpressure via poll_ready() method.

i dont see how Handle trait can replace Service trait.

@cetra3
Copy link
Contributor Author

cetra3 commented Jan 15, 2020

I'm not looking to replace the Service trait at all. The Handle trait is just simplified down for cases when you don't need backpressure or async initialization (like in a standard function/extractor sense).

I've been investigating upgrading one of my projects from 0.7 to 2.0 and found that it's going to be hard without an analog to the existing Handler trait & there is no real direction in the MIGRATIONS.MD file. I know there was a discussion that Service would be replacing the handler trait but that is much more complex than Handler was.

Another possibility is to look at using tower's service trait, but possibly behind a feature flag or as an external crate.

@fafhrd91
Copy link
Member

FnMut(ServiceRequest) -> Future<ServiceRespinse> is a service too

@cetra3
Copy link
Contributor Author

cetra3 commented Jan 16, 2020

Yep, but the challenge with that is it's a free floating function. If you could replace FnMut with a custom struct, then that would be a perfect solution

@fafhrd91
Copy link
Member

you can replace FnMut with any struct that implements Service trait.

@cetra3
Copy link
Contributor Author

cetra3 commented Jan 16, 2020

Yes you can, but not without having to define a ServiceFactory, otherwise there is no way to wire it up.

I.e, without this change this is the bare minimum to have a struct somewhere in your web app instead of a function.

Lots of boilerplate:

impl Service for TestService {
    type Request = ServiceRequest;
    type Response = ServiceResponse;
    type Error = Error;
    type Future = Ready<Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        Poll::Ready(Ok(()))
    }

    fn call(&mut self, req: ServiceRequest) -> Self::Future {
        ok(req.into_response(HttpResponse::Ok().body("Test")))
    }
}

impl ServiceFactory for TestService {
    type Request = ServiceRequest;
    type Response = ServiceResponse;
    type Error = Error;
    type Future = Ready<Result<TestService, ()>>;
    type Config = ();
    type InitError = ();
    type Service = TestService;

    fn new_service(&self, _config: ()) -> Self::Future {
        ok(self.clone())
    }
}

If there is a simpler way than that currently, I could not find it in documentation or examples or can be lead there by the compiler.

Compare that with the handle trait:

impl Handle for TestService {
    type Future = Ready<Result<ServiceResponse, Error>>;

    fn call(&mut self, req: ServiceRequest) -> Self::Future {
        ok(req.into_response(HttpResponse::Ok().body("Test")))
    }
}

27 Lines of code vs. 7 and a lot more readable.

@fafhrd91
Copy link
Member

You can use fn_factory

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.

None yet

2 participants