Skip to content

[NPM/YARN] Cache client-side timeouts when a remote host is unreachable#5373

Merged
brrygrdn merged 4 commits intomainfrom
brrygrdn/cache-timeouts-for-npm
Jul 14, 2022
Merged

[NPM/YARN] Cache client-side timeouts when a remote host is unreachable#5373
brrygrdn merged 4 commits intomainfrom
brrygrdn/cache-timeouts-for-npm

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Jul 13, 2022

Follows up on #5142

This PR modifies the timeout caching strategy we used for Maven into a generic 'Dependabot::RegistryClient' and then applies it to the npm_and_yarn gem to ship this strategy for another ecosystem.

NPM is currently the ecosystem experiencing this problem most often, so let's address it next.

Alternatives considered

I originally approached this as an Excon middleware, but there were a few issues with that approach in my opinion:

  • I'd prefer to roll out the change gradually one ecosystem at a time to avoid missing any unexpected interactions with package managers
  • I'd prefer to limit the change in Excon behaviour to code that is calling registries rather than all remote calls.
    • We could do this by parameterising the SharedHelpers to incorporate the middleware for some callers but not others, but that feels somewhat brittle
  • Since we are now caching state I'd prefer to do it within a concrete class, a middleware could be made to call out to a data store class but the lifecycle is slightly harder to reason about when it is part of an Excon stack

@brrygrdn brrygrdn requested a review from a team as a code owner July 13, 2022 13:25
Copy link
Copy Markdown
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

This approach makes sense to me 👍

Co-authored-by: Jurre <jurre@github.com>
@brrygrdn brrygrdn merged commit 7e44d40 into main Jul 14, 2022
@brrygrdn brrygrdn deleted the brrygrdn/cache-timeouts-for-npm branch July 14, 2022 10:46
@brrygrdn brrygrdn mentioned this pull request Jul 14, 2022
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