Make async fetching actually async #448
Merged
+862
−374
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This takes almost all our existing tests and converts them to dual-sync-async, testing that both ways of running requests returns the same results.
In the coming commits we'll move to actually async-ifying our "async" HTTP fetching logic. Here we start by making the response-reading utilities (which compromise the bulk of the logic) dual-sync-async by shoving them in a macro that gets run twice. Best reviewed with `--color-moved -b`
In the next commit we'll move to actually doing async reads when making an async request. However, in the current work we want to avoid changing the public API so that we can switch to real async without a new minor version. Thus, because we cannot convert `LazyResponse` to async, we have to ensure we complete all the async bits before returning a "`LazyResponse`", and thus have to pre-read the entire contents immediately. Here we implement this with a new `Response` builder, reading as required from an async stream.
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
from
5628c21 to
6993586
Compare
Prevously, "async" HTTP calls were actually just spawning blocking calls which were run in a background task and waited on. This is obviously rather un-scalable (not to mention breaks with single-threaded `tokio` executors) and can hang attempts to shut down the `tokio` reactor. Instead, here, we switch to doing actual async reads. In order to avoid changing the API (so that we can ship a point release), we still have to support returning a `ResponseLazy` from `send_lazy_async`, which we obviously can't make async since `ResponseLazy`'s read methods are sync. Instead, in that case we pre-load the full response and store the bytes in `ResponseLazy`, allowing it to read them as needed. Asking claude to copy `ResponseLazy` to an `AsyncResponseLazy` which reads from an actual async stream is left as an exercise for the reader. Fixes rust-bitcoin#441
Best reviewed with `--word-diff`
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
from
558f931 to
aab48de
Compare
tcharding
reviewed
Jan 8, 2026
Comment on lines
-51
to
+57
| pub fn create_secured_stream(conn: &Connection) -> Result<HttpStream, Error> { | ||
| pub(super) fn create_secured_stream(conn: &Connection) -> Result<HttpStream, Error> { |
Member
There was a problem hiding this comment.
Flagging that this module is private so this is not a breaking change.
Member
|
Would be nice to get a review from someone who knows async but since this crate is so new and I blindly vibe coded all the async stuff anyways I'm happy to just run ACK, merge, YOLO policy on this crate for now. @TheBlueMatt? |
Member
|
I'm going to merge this. The CI fails are unrelated (#447). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #441