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

feat(gatsby-core-utils): Add retry on HTTP status codes to fetchRemoteFile #33461

Merged
merged 6 commits into from
Nov 9, 2021

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Oct 7, 2021

We currently do not retry on certain status codes and network errors. This can cause issues when building projects that use a lot of remote assets that need to be fetched.

This replaces #30391.

Status:

  • add retry for error status codes
  • add retry for network errors
  • integrate with all plugins that use fetchRemoteFile

@axe312ger axe312ger marked this pull request as draft October 7, 2021 13:57
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 7, 2021
@axe312ger axe312ger marked this pull request as ready for review October 13, 2021 16:10
@axe312ger axe312ger changed the title feat: add retry to fetch remote file feat: add retry on HTTP status codes to fetchRemoteFile Oct 14, 2021
@axe312ger axe312ger requested a review from wardpeet October 14, 2021 18:44
@axe312ger
Copy link
Collaborator Author

@wardpeet as you suggested, I moved the logic from my wrapper into the error handler of the stream.

I struggle to add an actual timeout to the retry.

Also I am still not sure how and if we should simulate generic network errors, but for sure we should try to retry them.

\\"responseStatusCode\\": 503,
\\"responseStatusMessage\\": \\"Service Unavailable\\",
\\"requestHeaders\\": {
\\"user-agent\\": \\"got (https://github.com/sindresorhus/got)\\",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Out of scope for this.. but... what if we send a user-agent that contains GatsbyJS and the current version? Could imagine that would increase the visibility in logs, and might even help in support tickets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 22, 2021
@LekoArts LekoArts changed the title feat: add retry on HTTP status codes to fetchRemoteFile feat(gatsby-core-utils): Add retry on HTTP status codes to fetchRemoteFile Oct 22, 2021
@njbmartin
Copy link
Contributor

njbmartin commented Nov 4, 2021

This is sadly blocking us from being able to upgrade to Gatsby v4 as our build fails trying to download images from Contentful. Happens randomly but consistently after downloading 250+ images for us in the dev environment.

Edit: A fun read about unexplained connection resets in Docker - https://tech.xing.com/a-reason-for-unexplained-connection-timeouts-on-kubernetes-docker-abd041cf7e02

@axe312ger
Copy link
Collaborator Author

@njbmartin thank you for the link. This is currently our highest priority, I want to fix this in 2021!

@axe312ger
Copy link
Collaborator Author

axe312ger commented Nov 5, 2021

The old version of gatsby-source-contentful was reducing this issues by limiting the parallel downloads to 100 at the same time. It was not perfect, but drastically reduced the number of required retries.

@axe312ger axe312ger force-pushed the feat/retry-on-remote-fetch branch from 0922e59 to ea39580 Compare November 9, 2021 17:48
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 9, 2021
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Awesome! This fixes so many things! 🎉

@gatsbybot gatsbybot merged commit 00dc589 into master Nov 9, 2021
@gatsbybot gatsbybot deleted the feat/retry-on-remote-fetch branch November 9, 2021 19:05
@axe312ger
Copy link
Collaborator Author

Ohh yeah! :)

LekoArts pushed a commit that referenced this pull request Nov 10, 2021
…teFile` (#33461)

* feat: add retry to fetch remote file

* refactor to retry withing stream error event handler

* review changes

* fix tests

* add test and retry for network errors

* remove retry logger

Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit 00dc589)
LekoArts pushed a commit that referenced this pull request Nov 10, 2021
…teFile` (#33461) (#33925)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Co-authored-by: Benedikt Rötsch <axe312ger@users.noreply.github.com>
@LekoArts
Copy link
Contributor

Successfully published:
 - babel-plugin-remove-graphql-queries@4.1.1
 - babel-preset-gatsby@2.1.1
 - gatsby-cli@4.1.2
 - gatsby-core-utils@3.1.1
 - gatsby-page-utils@2.1.1
 - gatsby-plugin-benchmark-reporting@2.1.1
 - gatsby-plugin-gatsby-cloud@4.1.1
 - gatsby-plugin-image@2.1.1
 - gatsby-plugin-manifest@4.1.2
 - gatsby-plugin-mdx@3.1.2
 - gatsby-plugin-offline@5.1.2
 - gatsby-plugin-page-creator@4.1.2
 - gatsby-plugin-preload-fonts@3.1.1
 - gatsby-plugin-sharp@4.1.2
 - gatsby-plugin-typescript@4.1.1
 - gatsby-recipes@1.1.1
 - gatsby-remark-images@6.1.2
 - gatsby-source-contentful@6.1.2
 - gatsby-source-drupal@5.1.2
 - gatsby-source-filesystem@4.1.1
 - gatsby-source-graphql@4.1.1
 - gatsby-source-shopify@6.1.2
 - gatsby-source-wordpress@6.1.1
 - gatsby-telemetry@3.1.1
 - gatsby-transformer-remark@5.1.2
 - gatsby-transformer-sqip@4.1.2
 - gatsby@4.1.3

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…teFile` (gatsbyjs#33461)

* feat: add retry to fetch remote file

* refactor to retry withing stream error event handler

* review changes

* fix tests

* add test and retry for network errors

* remove retry logger

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants