Skip to content

Allow for reusing TCP connections#7

Open
jgaskins wants to merge 1 commit intoplace-labs:masterfrom
jgaskins:connection-pool
Open

Allow for reusing TCP connections#7
jgaskins wants to merge 1 commit intoplace-labs:masterfrom
jgaskins:connection-pool

Conversation

@jgaskins
Copy link
Contributor

@jgaskins jgaskins commented Feb 3, 2021

This eliminates TCP handshakes, TCP slow start, and potentially TLS negotiation for subsequent requests.

The max_idle_pool_size is just a starter value. I normally parse it from the URL the way the crystal-lang/crystal-db shard does, but I think it makes sense to discuss that as a feature first.

This eliminates TCP handshakes, TCP slow start, and potentially TLS
negotiation for subsequent requests.

The `max_idle_pool_size` is just a starter value. I normally parse it
from the URL the way the crystal-lang/crystal-db shard does[1], but I
think it makes sense to discuss that as a feature first.

[1] https://github.com/crystal-lang/crystal-db/blob/eaddae7d71d52536453ea2d94777854f1b057f83/src/db/driver.cr#L31-L40
@stakach
Copy link
Contributor

stakach commented Sep 28, 2021

thanks for this @jgaskins
I do like the idea of this however I am a little concerned by the possibility of the clients in the pool becoming disconnected and the pool not recovering
crystal-lang/crystal-db#154 (comment)

i think we'd need to implement retries and make sure to catch errors and raise DB::ConnectionLost to ensure things are cleaned up properly

@jgaskins
Copy link
Contributor Author

@stakach I thought HTTP::Client instances self-healing in those cases. Is that not true?

@stakach
Copy link
Contributor

stakach commented Sep 29, 2021

Looks like that was added recently, should drop in crystal 1.2 crystal-lang/crystal#11088

I've been having to deal with this manually by detecting closed sockets and creating new HTTP clients as required
https://github.com/PlaceOS/driver/blob/master/src/placeos-driver/transport/http_proxy.cr#L16

@jgaskins
Copy link
Contributor Author

jgaskins commented Oct 8, 2021

Oh interesting. I didn’t realize that hadn’t been implemented yet. I could’ve sworn I’d seen connections get closed and HTTP::Client would reconnect.

Ironically, I forgot to set Connection: keep-alive so it wouldn’t actually reuse the connection in its current state. 🙃 So it looks like there are a few things I need to update here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants