Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic connection cache #130

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

Timbus
Copy link

@Timbus Timbus commented Dec 19, 2020

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
Copy link
Author

Timbus commented Dec 19, 2020

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.

@hkalexling
Copy link
Member

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.

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