-
Notifications
You must be signed in to change notification settings - Fork 56
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
HTTP client options #467
Comments
@danschultzer Thanks for opening this issue; it's something I've thought about a lot recently. An adapter that uses httpc by default, with an easy switch to finch, is perfect. @joshuap I'll take this one. |
Sounds great @sorentwo, thanks for looking at this! I did make a quick PR in #468 to pass hackney opts which solves part of the issue. But it would be nice to just pass in an adapter to deal with edge cases and have more control over the connection egress. Just a warning on |
Is there still any interest in implementing this change? As of today hackney has a low-priority CVE with no fix listed (and no obvious fix activity on github). I'd love to swap it out for a different client if possible. |
@fastjames Yup! |
Well, it's good to know that someone else is interested as well. I read over some of the earlier links on this issue, and @danschultzer 's work in assent looked like it might work here too. I took a stab at shamelessly copying what he did, and I think I'm getting somewhere (see https://github.com/fastjames/honeybadger-elixir/tree/fastjames/feature/multi_http_client_support for my work so far). Important questions still to be answered:
|
I'm still interested in this change, but I dropped the ball implementing it. @fastjames Thanks for taking a shot at this. As to your questions:
It should be possible to use other adapters. A
Switching to |
Thanks for the feedback! Based on these answers, it sounds like the best course of action is to do the following:
|
I agree with your assessment, except for ensuring each supported client has a pool. As long as we ensure the default has a pool for backward compatibility, people should be able to plug in any other http adapter they like. WDYT? |
That's fair, and a good narrowing of scope (which I always like). Since |
I have opened the PR. I'm happy to squash it if everyone is OK with dropping |
I've experienced issues with honeybadger not working in a IPv6 only network. This is due to a long running issue in hackney of how IP addresses are resolved: benoitc/hackney#206 (in particular this logic).
Looking at how honeybadger deals with HTTP it would be good to:
this won't solve my particular issue thoughedit: should work)a. Switch to a flexible library like Tesla
b. Add an abstraction in honeybadger (this is how I handle it in https://github.com/pow-auth/assent/tree/main/lib/assent to support built-in httpc by default)
A third option, which is about solving the IPv6 issue, is to switch to something other than hackney since upstream fix seems unlikely to happen anytime soon. I solved my issue by switching to Finch and pass in
conn_opts
: https://github.com/danschultzer/honeybadger-elixir/commit/14bdbe21f6ea754c335478ccb82bb6fa312993a7The text was updated successfully, but these errors were encountered: