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

[Maven] Cache client-side timeouts when a remote host is unreachable #5142

Merged
merged 3 commits into from
May 17, 2022

Conversation

brrygrdn
Copy link
Contributor

@brrygrdn brrygrdn commented May 16, 2022

Maven projects tend to be fairly heavy on network activity as Dependabot attempts to recursively walk up the directory structure looking for parent pom.xml files when compiling a list of all maven registry hosts it should poll for version update data.

Once it has compiled this list, it will generally query these hosts three times per dependency. There may be some gains to be made in fully incorporating response caching for some very large projects but the issue that is causing most problems is that due to Dependabot's greedy registry detection if we find a registry that is behind a VPN, we will:

  • wait 5 seconds for a connect_timeout if running standlone
  • wait 20 seconds for a read_timeout if running behind a proxy, depending on it's implementation/configuration

The overall result is that detecting a VPN registry anywhere in the directory hierarchy of a project can delay each Dependency update by:

( Attempts x Timeout ) x 3 seconds

This amounts to up to ~1 minute per dependency standalone and ~6 minutes per dependency behind a proxy.

The Change

Dependabot uses Excon as a simple HTTP client and generally uses bare Excon.get calls using a set of defaults that are centrally maintained across all Ecosystems.

This PR introduces Dependabot::Maven::RegistryClient as a point of change local to our Maven support as:

  • I don't believe other ecosystems are likely to encounter the 'bystander' VPN registries
  • I don't want to introduce a RegistryClient pattern to core more generally (yet) 1

It only caches Excon::Error::Timeout as the specific signal of an unreachable host, there may be other responses that indicate an unhealthy host we should consider caching but I'm reluctant to do that in a first pass.

I choose this approach instead of an Excon middleware as I felt injecting it just for Maven was fairly indirect. If we decide this behaviour is something we should apply more generally, that might be a better approach.

Footnotes

  1. While looking at this I do wonder if it would be of benefit to materialise specific named clients for registries that are low tolerance vs high tolerance and introduce some general best practice behaviours for fault tolerance - consider; it is less of an issue failing to fetch release notes than failing to fetch an actual dependency.

@brrygrdn brrygrdn requested a review from a team as a code owner May 16, 2022 17:18
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

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