Skip to content

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

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Use async-http-faraday. #784

merged 1 commit into from
Apr 11, 2020

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Apr 9, 2020

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.

@ioquatix
Copy link
Contributor Author

I am now happy with this, we just need to rebase on #786 once it's merged.

@ioquatix
Copy link
Contributor Author

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.

@ioquatix
Copy link
Contributor Author

ioquatix commented Apr 10, 2020

Here is perf comparison:

fix-thread-unsafe

Generated log placed in /home/samuel/Documents/ioquatix/github-changelog-generator/lib/CHANGELOG.md
bundle exec ../bin/github_changelog_generator --no-http-cache -u  -p  -t   25.64s user 0.88s system 26% cpu 1:39.57 total

async-http-faraday (with faraday 1.0)

Generated log placed in /home/samuel/Documents/ioquatix/github-changelog-generator/lib/CHANGELOG.md
bundle exec ../bin/github_changelog_generator --no-http-cache -u  -p  -t   7.70s user 0.48s system 14% cpu 57.872 total

changelogs

They are the same

koyoko% shasum CHANGELOG*
390b735e55a97578f597d0919eedf3e14f43ea07  CHANGELOG-async.md
390b735e55a97578f597d0919eedf3e14f43ea07  CHANGELOG-threads.md

@ioquatix
Copy link
Contributor Author

Just for fun, here is master:

Generated log placed in /home/samuel/Documents/ioquatix/github-changelog-generator/tmp/CHANGELOG.md
bundle exec ../bin/github_changelog_generator -u github-changelog-generator -  26.77s user 1.08s system 30% cpu 1:31.09 total
koyoko% shasum *.md
390b735e55a97578f597d0919eedf3e14f43ea07  CHANGELOG-async.md
8091e439ec6528a1c50248721a8c68c0c238e31b  CHANGELOG.md
390b735e55a97578f597d0919eedf3e14f43ea07  CHANGELOG-threads.md

Copy link
Collaborator

@olleolleolle olleolleolle left a 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.

@ferrarimarco
Copy link
Contributor

@ioquatix can you rebase against the latest master?

@ioquatix
Copy link
Contributor Author

Rebased.

@olleolleolle
Copy link
Collaborator

Thanks again, merging.

@olleolleolle olleolleolle merged commit 7eef411 into github-changelog-generator:master Apr 11, 2020
@ioquatix ioquatix deleted the async-http-faraday branch April 11, 2020 11:17
@ekohl
Copy link
Contributor

ekohl commented Apr 11, 2020

I'd love to see this in a released version.

@ioquatix
Copy link
Contributor Author

Thanks for your enthusiasm!

@ioquatix
Copy link
Contributor Author

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.

@JacobEvelyn
Copy link

@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.

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.

Nondeterministic moving/deleting of PRs in CHANGELOG.md
5 participants