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

util: add Wrap #676

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

util: add Wrap #676

wants to merge 6 commits into from

Conversation

lilyball
Copy link

@lilyball lilyball commented Jul 2, 2022

This is middleware that wraps a service with both pre and post functions. It offers two flavors, one which is purely for passing shared state from the pre function to the post, and the other of which allows for asynchronous pre/post functions, mapping the request type, and short-circuiting so the inner service isn't called.

This is something I've wanted for a while, but it took a lot of experimentation to figure out how to actually express this in a way that was both generic and compiled. This is something that I always felt was missing functionality, because it turns out to be fairly common in my experience to want to take some service and wrap it in arbitrary middleware that needs to share state across the call to the inner service.

Some common uses of this are to copy data from the request to the response or to stick a cache in front of a service.

I do think this could probably stand to have some examples added to demonstrate using it, but I didn't want to make the documentation too complex. I'm also open to the idea of flipping the names of decorate and wrap, I just figured that wrap was the more obvious name to go for and so that should be the one with simpler usage.

This is middleware that wraps a service with both pre and post
functions. It offers two flavors, one which is purely for passing shared
state from the pre function to the post, and the other of which allows
for asynchronous pre/post functions, mapping the request type, and
short-circuiting so the inner service isn't called.
@lilyball
Copy link
Author

lilyball commented Jul 2, 2022

More succinctly, this handles the problem of wrapping a service with service_fn() not handling backpressure from the wrapped service.

Also tweak nested type signatures on `Pre`/`Post` to use type parameters
instead of associated types to simplify the rustdoc output.
The `ServiceExt::wrap` combinator was supposed to let the post function
return a different result type than the inner service, but this wasn't
properly expressed in the type system.
@lilyball
Copy link
Author

lilyball commented Jul 11, 2022

FWIW I just prototyped an alternative solution that looks like

fn service_fn2<T, CallF, PollF>(state: T, poll_ready: PollF, call: CallF) -> ServiceFn2<T, CallF, PollF> {}

impl<T, CallF, PollF, Fut, Request, R, E> Service<Request> for ServiceFn2<T, CallF, PollF>
where
    CallF: FnMut(&mut T, Request) -> Fut,
    PollF: FnMut(&mut T, &mut Context<'_>) -> Poll<Result<(), E>>,
    Fut: Future<Output = Result<R, E>>,
{}

and while it's technically doable, we get really awkward errors with the compiler and have to write out type signatures everywhere when using it. I assume this is at least partially due to not being able to specify bounds in the service_fn2 signature, but adding bounds there constrains the function (in particular, it would prevent using this to return a service that is generic over its request type, which may be a problem if you're trying to wrap an inner service that is generic).

Edit: I suppose producing a generic service with this doesn't work anyway as closures cannot be generic. The Fn family in theory allows for generic implementors but I'm not aware of any way to achieve this today on stable since you cannot implement them manually on a type. But I wouldn't be surprised if other issues crop up when hoisting the constraints up to the function.

I also wrote up an example for wrap that works okay, but ran into an issue when writing one for decorate which is that I can't share state between the two without having to coordinate mutation (something like Rc<RefCell<T>> or Arc<Mutex<T>>). I was hoping the Pre function could return a mutable reference as its shared state but it turns out you can't return mutable references to captured values in FnMut (I guess the FnMut trait can't represent that lifetime). So I'm wondering if maybe it's worth adding a separate state parameter to decorate that gives you something that's passed to both functions, or if that's just going to be complex enough that it's not really a win over making a bespoke Service impl.

Edit 2: Oh right, if we add mutable state then it needs to deal with Arc<Mutex<…>> anyway because this whole thing is async.

@lilyball
Copy link
Author

I pushed examples for wrap and decorate (along with some other fixes). wrap is a nice simple example, but I'm not sure how compelling the decorate example is. I just know that trying to add a cache in front of a service was something I really struggled with before (and ended up writing a custom service for before writing decorate) and it feels like something that should be possible without writing custom types.

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

1 participant