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

Leverage the tower ecosystem for http requests #934

Closed
zecakeh opened this issue Aug 8, 2022 · 12 comments
Closed

Leverage the tower ecosystem for http requests #934

zecakeh opened this issue Aug 8, 2022 · 12 comments
Assignees
Labels
help wanted Interested in working on the project? These are great additions we'd like to have! needs-info Further information is requested

Comments

@zecakeh
Copy link
Collaborator

zecakeh commented Aug 8, 2022

From @sandhose in the Matrix room (permalink):

I've been discussing with @zecakeh for the OIDC client implementation in the Rust SDK, and reusing some of the work I've done in matrix-authentication-service. In MAS, I'm leveraging the tower ecosystem when I'm doing http requests. Instead of using a high-level client like reqwest, I'm using a tower::Service<http::Request, Response = http::Response>.

This allows me to have things like injecting headers, tracing, retries, timeouts, etc. implemented via tower layers. It's pretty powerful IMO, so I wondered if it would make sense to have the same approach in the Rust SDK.

I see that in the Rust SDK, you defined an HttpSend trait, which has an async fn send_request(&self, request: http::Request<Bytes>, config: RequestConfig) -> Result<http::Response<Bytes>, HttpError>, which is pretty close of what an tower::Service<http::Request<Bytes>, Response = http::Response<Bytes>, Error = HttpError> would provide. What would you think of switching to that instead of the HttpSend crate? That would help leverage many great things from the tower ecosystem

@gnunicorn gnunicorn added the help wanted Interested in working on the project? These are great additions we'd like to have! label Aug 18, 2022
@Hywan Hywan self-assigned this Aug 18, 2022
@Hywan
Copy link
Member

Hywan commented Aug 18, 2022

Hello,

We have discussed about your proposal in our weekly meeting. We think it could be a good idea to move to Tower as it won't add too much dependencies and weight to the project, in comparison to the features it can provide. The main 2 features we see is supporting timeouts and retries.

Sadly, we don't presently have the bandwidth to implement that though. Would you be willing to open a PR to update the project? We suspect you've a good knowledge of the HttpSend trait and of Tower. We will be happy to review the PR and doing mentorship if needed. How does it sound for you?

@Hywan Hywan added needs-info Further information is requested help wanted Interested in working on the project? These are great additions we'd like to have! and removed help wanted Interested in working on the project? These are great additions we'd like to have! labels Aug 18, 2022
@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 18, 2022

Sure, I'll work on that when I have the time and open a PR

@Hywan
Copy link
Member

Hywan commented Aug 22, 2022

Thank you! Feel free ping to me anytime.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 24, 2022

This is currently blocked because reqwest's Client doesn't implement Service on WASM.

@jplatte
Copy link
Collaborator

jplatte commented Aug 24, 2022

Maybe it's just time to find another way to do requests on wasm then. reqwest has always been pretty incomplete in annoying ways on wasm.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 24, 2022

What do you have in mind? Create our own abstraction around the Fetch API (maybe using gloo-net)? Find another library that has better support on all platforms?

@jplatte
Copy link
Collaborator

jplatte commented Aug 24, 2022

gloo-net plus tower integration seems like a reasonable solution. Although it's unfortunate that integration doesn't already exist in some other crate (AFAICT).

@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 24, 2022

I'm not surprised, the built-in tower Layers don't seem to try to be compatible with WASM. The ones we want to use, timeout and retry, both seem to depend on tokio/time according to the Cargo manifest, which is not compatible with WASM, afaict.

We'll probably need to implement our own Layers.

@jplatte
Copy link
Collaborator

jplatte commented Aug 24, 2022

Well maybe tokio/time can be made compatible with wasm, or tower can add some other dependency for those middlewares. It would be unfortunate to reimplement them entirely.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 24, 2022

Actually it seems like there's been work on support for WASI in tokio that should be in the next release and in this follow-up issue there's a "tokio::time can be stabilized." todo.

We should figure out if it also improved support for the wasm32-unknown-unknown target.

@Hywan
Copy link
Member

Hywan commented Aug 29, 2022

If we need WASI support, we can compile Wasmer to WebAssembly, to then execute Wasm modules with Wasmer in a browser for example. It works pretty well. But it's possibly overkill for what we need.

For what I know, WASI has no official support for networking API yet (👀 wasi-io). I was working on an experimental API months ago (even with Zig support & co., that was cool), but maybe I've missed some news in that area. There is no support for TcpListener::bind nor TcpStream::connect yet in Tokio (and WASI in general for what I know), which makes things quite limited for our usecase.

I would suggest to just have another strategy for Wasm for now. This topic is going to be long to converge to a single solution.

@Hywan
Copy link
Member

Hywan commented Aug 10, 2023

I don't think we are going to move this forward for the moment :-). Let's close it.

@Hywan Hywan closed this as completed Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Interested in working on the project? These are great additions we'd like to have! needs-info Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants