feat(tonic): introduce async_interceptor

[jdahlq]

Motivation

See Issue #870.

Tonic server provides interceptor for easy creation of a middleware layer from a closure. This interceptor can inspect and transform the headers and metadata of a request, as well as rejecting it by returning a status.

This PR introduces async_interceptor, a version of interceptor that accepts async functions and closures, allowing for the creation of more powerful middleware without the usual Tower boilerplate.

Solution

The code for interceptor was duplicated and adapted for use with an async interceptor. I was initially hoping to reuse most of the functions which support interceptor, but it would have required heavy modification of those functions and possibly less performant code, so ultimately I decided it was better to leave that code mostly untouched.

I found it challenging to write clean code without using async/await or FutureExt functions like then which chain futures together. As far as I can tell, those features would have required boxing the returned futures, and I'm assuming that we want to keep the futures in the core library on the stack, which this PR manages to do.

I also added some tests and updated the Tower example.

I'm still rather new to Rust (coming from a mostly Java and C++ background), so I'd really appreciate advice on code style. Thanks!

[jdahlq]

TODO: Update this PR to avoid the CORS issue described in Issue #911 once PR #912 is submitted.

[LucioFranco]

This feels almost exactly like the tower service trait, what is the advantage and why don't we just use the tower traits?

[jdahlq]

Hi @LucioFranco , sorry for letting this PR sit for so long. I've merged the latest master into my branch, updated the code to work with those changes, and refactored a bit to reduce code duplication.

Note that I had to constrain ReqBody to implement Default. Seems like that should be ok (and ResBody already had that constraint).

In response to your question...

I would love to use something in Tower rather than create all of this gnarly generics+futures code, but I couldn't find anything that will do the trick. You could ask the same thing about the non-async interceptor, too (written by @davidpdrsn ?). I think the main reason is that Tower doesn't have anything that both (a) modifies a request and (b) allows an error response to be returned immediately.

Tower has service_fn which makes an async function into a Tower Service, but it doesn't wrap an inner service, so I don't think it can be used as a middleware layer.

Looking at the tower::util::ServiceExt trait, there are are some promising methods that don't quite fit the async_interceptor case:

• map_request transforms the request and wraps the service, but it doesn't accept async functions, and it cannot skip calling the wrapped service.
• filter_async is doesn't transform the request, but is otherwise close to what we want.

I also spent quite a bit of time trying to simplify this code with async functions, but since their return types are generated at compile time, it's hard to make them play nicely with the Service trait since it requires explicitly specifying the Future associated type, and we can't set Future = impl Future<Output = ...> until this rust RFC is accepted and shipped.

[davidpdrsn]

The main advantage is that this uses tonic's Request and Status types. Whereas tower middleware have to use the types from http.

[jdahlq]

The interop tests for windows-latest failed, looks like the test is just hanging? @LucioFranco maybe just needs to be re-run?

[LucioFranco]

Restarted that test

[jdahlq]

It's a prost-build error:

CMake Error at CMakeLists.txt:24 (project):
The CMAKE_C_COMPILER:

  C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.31.31103/bin/Hostx64/x64/cl.exe

is not a full path to an existing compiler tool.

Probably isn't caused by this PR. I'm assuming it's related to: tokio-rs/prost#620

[jdahlq]

It passes now, I think it was just flaky or a dependency issue. I merged the latest (unrelated) commit just to re-trigger the tests.

[UnsolvedCypher]

Hi @LucioFranco, this looks like it is complete and checks are passing, can it be merged now?

[LucioFranco]

I will try and give this a review in the coming weeks I have a decent amount of backlog right now.

[jdahlq]

Hi @LucioFranco , it's been a few months, but you seem to have a lot on your plate, and I understand that this merge request is relatively unimportant. Please let me know if realistically speaking this won't be merged so I can put it in its own crate or something and don't have to rely on a forked version of tonic. Thanks!

[LucioFranco]

At this point I don't have much time for this and in general tonic will be moving to be lighter (will eventually post the roadmap when I finish it lol). So if you can move this to your own crate for now that would probably be the easiest path forward. Sorry for the delay.

[jdahlq]

At this point I don't have much time for this and in general tonic will be moving to be lighter (will eventually post the roadmap when I finish it lol). So if you can move this to your own crate for now that would probably be the easiest path forward. Sorry for the delay.

Got it, I'll give that a try and post a link here for people who are interested.

[jdahlq]

Hey Lucio, would you consider a few small function visibility changes if I sent you a new merge request?

Currently async_interceptor relies on a number of non-public functions. I can work around that except for one place: Neither Request::into_http nor Extensions::into_http are public. Either one being public is sufficient. Note that Request::from_http is already public, so it's not too much of a stretch to do the same for into_http.

For proper error handling, we also need body::boxed to be public because it calls a long chain of non-public functions.

Finally, it would be nice if Request::from_parts, and Request::into_parts were public, but it's not strictly necessary.

Thanks for your time!

[jdahlq]

Here is the minimal set of changes (3 lines) that I need to make an external async_interceptor crate work:
6af8bde

If that is acceptable, I'll open a merge request.

[LucioFranco]

@jdahlq the into_http I think we can merge but the boxed one I believe we shouldn't expose that method maybe but we can expose stuff on Status to let you compose your own body type. That to me feels more flexible and less exposing internals about the body. What do you think?

[jdahlq]

@LucioFranco yes, I agree it would be a little weird to exposed boxed, especially since it is doing more than just boxing. Are you thinking we could expose Status::from_error? That seems like a reasonable part of the API to me, and it would be sufficient for my purposes.

[kwiesmueller]

@jdahlq do you already have a repo with the standalone interceptor?
I'd be interested in helping test it for an auth interceptor.

[jdahlq]

Hi @kwiesmueller , you can grab the code here: https://github.com/arcanyx-pub/tonic-async-interceptor

It is published as tonic-async-interceptor and currently points to a forked version of tonic, so you can use it in your Cargo.toml like this:

[dependencies]
# ...
tonic = { package = "tonic-arcanyx-fork", version = "0.8.1-alpha.0" }
tonic-async-interceptor = "0.1.0-alpha.0"
# If you use tonic-web, you'll also have to use a forked version:
tonic-web = { package = "tonic-web-arcanyx-fork", version = "0.8.1-alpha.1" }

The forked versions are taken from the most recent commit on hyperium/tonic:master.

[jdahlq]

@kwiesmueller and all, I have published v0.1.0 of tonic-async-interceptor which uses tonic v0.8.1. Hope it is useful!

[LucioFranco]

Thanks for getting this rolling outside of tonic, I appreciate that! I think we can close this PR for now.