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

replace reqwest with hyper/hyper-util for xmtp_api_http in create_grpc_stream #997

Open
insipx opened this issue Aug 27, 2024 · 1 comment · May be fixed by #1444
Open

replace reqwest with hyper/hyper-util for xmtp_api_http in create_grpc_stream #997

insipx opened this issue Aug 27, 2024 · 1 comment · May be fixed by #1444
Assignees
Labels

Comments

@insipx
Copy link
Contributor

insipx commented Aug 27, 2024

we have a crate, xmtp_api_http that is a drop-in replacement for xmtp_api_grpc in the case that we need to use xmtp_mls on the web. This crate is generally working, and all tests in xmtp_mls pass.

Reqwest keeps a connection pool alive when using their library, in order to re-use http connections. For our streaming POST requests in xmtp_api_http, we expect the connection to stay alive for the entirety of the request. Reqwest actually spawns a future when the request is sent to watch the request, and re-add the connection to the pool once the request ends. When used with a single-threaded executor, this blocks execution from progressing, since the POST request we're using is long-lived and doesn't end, the two futures mutually keep each other alive and prevent the other from finishing.

We can fix this by using hyper (the lower-level library reqwest is built upon), and crafting the POST request manually & using a separate connection from reqwest. This will allow our streams to be used in current_thread and prevent blocking the executor.

Relevant tests: test_stream_messages, test_stream_conversations

Note: WebAssembly uses a different connection pooling/stream strategy than native reqwest, so it's to be seen if this is a blocker for webassembly support.

rel: seanmonstar/reqwest#500

@insipx insipx added this to libxmtp Aug 13, 2024
@insipx insipx converted this from a draft issue Aug 27, 2024
@insipx insipx added good first issue Good for newcomers enhancement New feature or request medium needs-elaboration and removed good first issue Good for newcomers labels Aug 28, 2024
@insipx insipx changed the title Use custom Pool & replace reqwest with hyper/hyper-util for xmtp_api_http replace reqwest with hyper/hyper-util for xmtp_api_http in create_grpc_stream Sep 10, 2024
@insipx
Copy link
Contributor Author

insipx commented Sep 10, 2024

Just a bit more context:

The tests I'm talking about are here:

async fn test_stream_welcomes() {

using hyper for the streams should allow the test linked and the one below that to complete without tokio::spawning a new task for the stream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
2 participants