Skip to content

Conversation

@karlseguin
Copy link
Collaborator

@karlseguin karlseguin commented May 11, 2025

The idle connection pool is currently sub-optimial, as it only re-uses connections on an exact match. Specifically, it would be nice if a connection could be re-used if the host:port is a match, but the current implementation also requires the blocking mode to match.

So we potentially still need 4x more connections, but in reality, it's probably more like 2x. Most people are only going to be dealing with TLS traffic, so when you connect to lightpanda.io, you'll need 1 connection for the blocking calls and 1 connections for the nonblocking calls.

For plaintext request, it would be simple to share connections between blocking and nonblocking. But for encrypted requests, we need to dig more into tls.zig and figure out how/if a tls_client can be converted to <-> from blocking to nonblocking (they're different types).

@krichprollsch krichprollsch requested a review from mookums May 12, 2025 12:31
@mookums
Copy link
Contributor

mookums commented May 12, 2025

Looks fine to me.

  1. Just a thought but iIt would be nice if we could have some sort of before and after performance figures considering that keep-alive can have a pretty large impact on request/response times.
  2. Not sure if it's applicable here but the last time I had to work with a socket that needed to be TLS secured or bare, I ended up just using a vtable that just wraps the socket and provided like some base functionality (like a Reader and Writer) and all of the magic was opaque to the caller. It helped make a lot of the logic simpler as you just had to focus on the functionality rather than maintaining two implementations side by side.

@karlseguin karlseguin force-pushed the http_client_keepalive branch from 3d6e699 to 64f8031 Compare May 13, 2025 02:43
@karlseguin
Copy link
Collaborator Author

The e2e workflow does a perf test, but you don't see much of a difference. The keepalive on localhost without TLS doesn't really add anything. But I can say that anecdotally, it is a noticeable improvement when I'm trying to work on duckduckgo supprot.

For both the synchronous and asynchornous handlers, I'm using a union to deal with the differences between TLS and plaintext. I think a union is better than an interface when the implementations are known at compile time, no? But, for async, it's very different..not just the APIs, but also the flows..with TLS handshake requiring extra round trips

@karlseguin karlseguin merged commit 7bb6506 into main May 13, 2025
9 checks passed
@karlseguin karlseguin deleted the http_client_keepalive branch May 13, 2025 02:51
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants