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 https://github.com/hyperium/tonic/issues/911 once PR https://github.com/hyperium/tonic/pull/912 is submitted.

[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: https://github.com/tokio-rs/prost/pull/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: https://github.com/hyperium/tonic/commit/6af8bde4bb41d3bdd265c8c6a56ad7fdd9302cac

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.