state: move instantiation into State::from_request

[krallin]

This refactors the service implementation a little bit to instantiate
the State via a dedicated method. The underlying goal is to allow users
to use Gotham without having to use its handler.

The motivation for this is to let users inject more things into their
state and call their handler directly instead of having to go through a
Hyper service. In our case, we use this for:

• Integrating in a Hyper service for a sub-route.
• Injecting data about TLS client certificates that were used by the client

[krallin]

The clippy errors that are failing in CI look to be pre-existing here

[codecov]

Codecov Report

Merging #526 (c90fcc5) into master (54b81e7) will decrease coverage by 0.53%.
The diff coverage is 37.87%.

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
- Coverage   85.54%   85.00%   -0.54%     
==========================================
  Files         109      110       +1     
  Lines        5577     5617      +40     
==========================================
+ Hits         4771     4775       +4     
- Misses        806      842      +36     

Impacted Files	Coverage Δ
gotham/src/lib.rs	83.33% <ø> (ø)
examples/custom_service/src/main.rs	2.70% <2.70%> (ø)
gotham/src/state/mod.rs	86.88% <80.76%> (-4.55%)	⬇️
gotham/src/service/mod.rs	100.00% <100.00%> (+9.43%)	⬆️
gotham/src/service/trap.rs	99.06% <100.00%> (ø)

---

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 54b81e7...9211322. Read the comment docs.

[msrd0]

Thanks for the PR!

I'm not sure I like everyone being able to create instances of State though ... so far, the only ways to obtain a State is either through a handler or through State::with_new. The first one gives you an owned version but requests it back as the return type of the handler, the second one only passes your callback a mutable reference. As long as we don't force all handlers to take mutable references, which will have to wait at least until async closures are properly supported on stable rust, I don't think I want any way to obtain an owned version of the State that is not made inaccessible through pub(crate) or made temporary as in you have to return its ownership back to gotham.

If I understand your use case correctly, you want to call a gotham handler from a raw hyper response and not use gotham to interface with hyper. This should be possible if you changed your handlers to take mutable references to the state, and converted the from_request method to one that takes an already existing state and only populates it with the information from the request. This way, gotham could internally call let mut state = State::new(); state.populate_from_request(request); and you could use State::with_new to call your own handlers. (Feel free to come up with a better name than populate_from_request)

And don't worry about the clippy errors, that check is marked optional but GitHub doesn't really display that anywhere.

[krallin]

If I understand your use case correctly, you want to call a gotham handler from a raw hyper response and not use gotham to interface with hyper

Not quite — let me rephrase. I have a hyper::Request + some extra context I extracted from the client's socket that I want to throw into the state. Then, I just want to call this method on my existing Gotham Handler:

https://github.com/gotham-rs/gotham/blob/master/gotham/src/handler/mod.rs#L167

That method doesn't really take a &mut State, but even if it it did, I don't think I could use State::with_new. If Gotham gives me a borrowed state, my return value's lifetime can't exceed the lifetime of the borrowed state. Unfortunately, my return value is a future that needs to be 'static.

Here's what my code looks like. This might help:

impl<H: Handler + Clone + Send + Sync + 'static + RefUnwindSafe> MononokeHttpHandlerAsService<H> {
    pub async fn call_gotham(self, req: Request<Body>) -> Response<Body> {
        let mut state = State::from_request(req, self.addr);
        if let Some(tls_socket_data) = self.tls_socket_data {
            tls_socket_data.populate_state(&mut state);
        }

        let res = match self.handler.handle(state).await {
            Ok((_state, res)) => res,
            Err((state, err)) => err.into_response(&state),
        };

        res
    }
}

// This just a little bridge to Hyper for places where we want to use this directly in `serve_connection`.
impl<H: Handler + Clone + Send + Sync + 'static + RefUnwindSafe> Service<Request<Body>>
    for MononokeHttpHandlerAsService<H>
{
    type Response = Response<Body>;
    type Error = anyhow::Error;
    type Future = BoxFuture<'static, Result<Self::Response, Self::Error>>;

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

    fn call(&mut self, req: Request<Body>) -> Self::Future {
        let this = self.clone();
       
        async move {
            let res = this.call_gotham(req).await;
            Ok(res)
        }
        .boxed()
    }
}

(MononokeHttpHandlerAsService is basically your ConnectedGothamHandler, except with the things I want and not the things I don't want :) )

Can you share a little more about the reasons behind the objection to have an owned state exposed to the client?

Thanks!

[msrd0]

Ok I think I understand what you're trying to acchieve - still I think my suggestion would work, you'd just have to define your own trait for a handler that takes &mut State, similar to AsyncHandlerFn.

As to not allowing users to create an owned version of State - this has been impossible before I've gotten involved in gotham, so I don't now the original reasoning behind State::new being private. I'd assume the reason is to ensure that all user-supplied handlers return the state they were passed initially, and don't create a new state to return from the handler, destroying the state they were passed as argument. At least I think this is a good reason to disallow the creation of owned states outside of gotham's internal code.

[krallin]

Ok I think I understand what you're trying to acchieve - still I think my suggestion would work, you'd just have to define your own trait for a handler that takes &mut State, similar to AsyncHandlerFn.

But then I can't use a Gotham Router as my handler ... so I'm not using Gotham anymore? Am I missing something?

[msrd0]

Oh yeah you're right that won't work ... what you're trying seems related to #183, right? I have never had this problem before so I'm not sure what the best way to solve this is. Having State::new private and State::from_request public seems weird at first, but might make sense since it makes it a little bit harder to return a newly created state from a handler instead of the original one.

I'm just trying to keep the api straight-forward, and returning (State::new(), create_response(...)) seems straight-forward but is incorrect, so I want to make sure nobody thinks something like this would be correct.

[krallin]

Oh yeah you're right that won't work ... what you're trying seems related to #183, right? I have never had this problem before so I'm not sure what the best way to solve this is

Yes, pretty much. I think one alternative here is to have Gotham pass through Extensions and make the "connect" mechanism a public interface, then I can take inject the extra state I have from Extensions into the Gotham state early in my middleware stack (this is what I mentioned on your other PR where I suggested maybe we shouldn't pull out just the OnUpgrade from extensions).

To avoid making the Connected variant public, and address the use case in #183, Gotham could have 2 hyper::Service implementations. The "connected" one can just inject the client address into extensions (and stay private), and the "not connected" one can just take them from Extensions, then callers can then add their own client address into extensions. Gotham might have to tolerate the client address being missing though.

Thoughts? Would you be more amenable to that kind of patch?

[msrd0]

No I think exposing State::from_request might be the best way to support this, and letting users create their own Service implementation if the one gotham provides does not fit their needs. However, this means that there are no panic handlers by default for custom services, I'm not sure if this is desirable. Do you think it would make sense to make gotham::service::trap::call_handler public? You could then use it instead of self.handler.handle(state) in your call_gotham method.

Regarding the code, could you add a small example (similar to the one you gave above) that demonstrates how to use gotham with a custom service? Also, please move the from_request method to the top below the with_new method, I'd prefer factory methods to appear before instance methods in the impl block.

[krallin]

Do you think it would make sense to make gotham::service::trap::call_handler public? You could then use it instead of self.handler.handle(state) in your call_gotham method.

I have no objections at all, though at least as far as I'm concerned I'm not really planning to use it (we configure all our services to abort on panics). I'm happy to fold this into this change.

Regarding the code, [...]

Yeah, no problem, I'll update the PR with this. Thanks!

[krallin]

I updated the PR with an example and moved the code around. Let me know if I should also make the trap handler public and perhaps use it in the example. Thanks! :)

[msrd0]

Let me know if I should also make the trap handler public and perhaps use it in the example.

Yes, please do. This should make the example service behave exactly as the gotham provided service, thus should be a good starting point for everyone that needs to customize their service.

[krallin]

Yes, please do. This should make the example service behave exactly as the gotham provided service, thus should be a good starting point for everyone that needs to customize their service.

Cool; I updated the PR to use call_handler :)

[msrd0]

Thanks

[krallin]

@msrd0 :do you think we could get a release with those changes at some point? I'd love to get us off of our fork :)

[msrd0]

I'm currently waiting on #468, after that I'll get a release out.

[msrd0]

FYI: I just released version 0.6.0