-
-
Notifications
You must be signed in to change notification settings - Fork 849
Use async-http-faraday
.
#784
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
Use async-http-faraday
.
#784
Conversation
I am now happy with this, we just need to rebase on #786 once it's merged. |
Also, this branch is about 20-40% faster depending on what you are doing, and there is other low hanging fruit to improve perf but I don't have any more time for this right now, so once it's merged we can look at improving performance further. |
Here is perf comparison: fix-thread-unsafe
async-http-faraday (with faraday 1.0)
changelogsThey are the same
|
Just for fun, here is master:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ This is a helpful and maintainable change which uses Faraday middleware and adapters to support GitHub Changelog Generator's use-case.
Thanks for offering this!
LGTM.
@ioquatix can you rebase against the latest master? |
Rebased. |
Thanks again, merging. |
I'd love to see this in a released version. |
Thanks for your enthusiasm! |
When I made this PR, I didn't know how to efficiently reuse the persistent connections because it was not possible to scope resources to the outer reactor. But now that it is possible using transient tasks, there is some more performance to be had. We don't need to close the persistent internet connection after every block of requests. This is something we should revisit. |
@olleolleolle I'm not sure if you have the ability to do this, but could we get this released in a new RubyGems version? Without this fix the gem is known to produce nondeterministic results (see #774) so it's unusable for me. |
This is an alternative to #729
Be aware that until sourcelevel/faraday-http-cache#118 is released,
async-http-faraday
is unable to use persistent connections correctly because that dependency pulls in an old release of faraday.In addition, I had to add a monkey patch because the Sawyer agent doesn't expose any way to close the underlying persistent connection, and we are waiting on lostisland/sawyer#67 to be merged/released.
Fixes #774.