Mango icon indicating copy to clipboard operation
Mango copied to clipboard

Add basic connection cache

Open Timbus opened this issue 5 years ago • 2 comments

So yeah, this just throws each HTTP::Client in a hash and reuses it if the host matches. I guess you might want to purge the cache in the future? But you'd have to be running it for months to ever really matter..

I switched url to a URI to make things a little easier, but I guess you could just do that locally in cached_client if you prefer it as a String

Timbus avatar Dec 19 '20 08:12 Timbus

Aw, so after a few hours of testing I noticed one.. issue. The HTTP::Client retry logic is broken: https://github.com/crystal-lang/crystal/blob/master/src/http/client.cr#L585

The client is supposed to reconnect when the server disconnects, but it looks like that code throws an exception and doesn't catch it.. Could be due to the HTTPS wrapper, it's been the culprit in the past. That or the library hasn't kept up with the socket implementation.

So yeah, I might make a PR to crystal-lang to fix this, before you'd wanna merge this.

Timbus avatar Dec 19 '20 10:12 Timbus

Thanks a lot for the PR! It looks good to me so far. Yes, it would be great if you could take your time and fix the upstream issue as well 👍

Note to future self: We have a few monkey-patches for HTTP::Client (this and this), and I would need to rewrite them so they work with instantiated clients.

hkalexling avatar Dec 19 '20 12:12 hkalexling