[bitreq] Support connection reuse and clean up Connection
#450
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
TCP ports are 16 bits, not 32 bits.
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
from
7b58f13 to
77b113f
Compare
The design of the `Connection` and `AsyncConnection` makes no sense. For some reason a "connection" owns a "request"? Worse, the connection isn't actually built until the `send`/`send_https` method is called, not when the `Connection` object is built. This also causes some code duplication in the `send` and `send_https` methods. In coming commits we want to move to reusing connections, so really need to start by cleaning this mess up. Here we just tackle the async connection, moving to establishing a TCP/TLS stream in the constructor and only sending the HTTP request in `send`. This also allows us to remove the difference between `send` and `send_https`. We also go ahead and define a `ConnectionParams` struct which we'll use for our key to determine if a connection can be reused across requests/redirects. Fixes rust-bitcoin#444 for the async case.
Writing the first bytes to a socket should generally not fail, but it is possible that it does in rare races involving immediate connection resets (or massive proxy parameters). Thus, we should `?`, not `unwrap`.
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
3 times, most recently
from
985d304 to
097ae82
Compare
This is the previous commit repeated for the sync `Connection`. Fixes rust-bitcoin#444 in the sync case. Written by Claude.
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
3 times, most recently
from
03f5d39 to
85c1ca2
Compare
In coming commit(s) we'll add handling for `Connection` and `Keep-alive` headers, so here we go ahead and start including them randomly in tests.
If we receive a redirect response to something on the same domain/HTTPS setting, we don't need to open a whole new connection, we can just reuse the existing one we have. Further, eventualy we want to support reusing connections across requests, which will require supporting reuse in `AsyncConnection`. This prepares for that support in `AsyncConnection`, starting with redirect support where we reuse the existing connection if possible, but the implementation details should allow for full `AsyncConnection` reuse generally. We design the logic to also support pipelining in a future commit with relatively few changes.
Initiating a connection (especially a TLS one) requires a nontrivial number of round-trips. Thus, its nice to be able to avoid it in cases where we just made another connection. In a previous commit we did so for redirects, here we do so across requests by introducing a new `Client` struct. Because we cannot `split()` the sync TLS/TCP streams, this is only implemented for async clients. Fixes rust-bitcoin#442
While HTTP/1.1 pipelining gets a bad name (some ancient servers handled it poorly), its quite useful to optimize RTTs when requesting from a server you know isn't garbage (i.e. anything from the last decade or two). Thus, here, we enable support for pipelining requests, however leave it opt-in.
TheBlueMatt
force-pushed
the
2026-01-asyncify
branch
from
85c1ca2 to
30a7d38
Compare
Member
Author
|
cc @tnull in case you want to take a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
I went into this thinking it would be easy and claude would be able to do it. I was very, very wrong. The
Connectionstuff was an absolute mess and basically had to be rewritten, but then actually properly supporting pipelining requests was more annoying that I thought. Still, it works now, and I even tested it in BDK's rust-esplora-client inside of ldk-node, which hasn't yet broken. Probably best to reviewconnection.rsfrom scratch as of the third-to-last commit and as of the end and not bother looking at the earlier intermediate commits inconnection.rs.