fix(gatsby-source-filesystem): Removes erroneously added retries option

[ThyNameIsMud]

Specifies remote timeout to start after the connection has been made

Description

gatsby-source-filesystem's remote file node will occasionally throw a got.TimeoutError if the TCP socket was queued. The solution was to target the specific send timeout which starts when the socket is connected and ends with the request has been written to the socket. This keeps with what I assume is the intended purpose of the timeout while hopefully throwing the error simply because the TCP connection was queued.

This also removes the retries option from the got request since the docs note streams ignore this option due to the errors that would cause a retry would need to be handled correctly to avoid duplicates (which they are). Note: The got.TimeoutError is a separate timeout and wouldn't be triggered here.

Documentation

https://github.com/sindresorhus/got#gottimeouterro
https://github.com/sindresorhus/got#retry

Related Issues

Fixes #22010
Fixes the got part of #23123

[pieh]

You are linking to docs for most recent got version, but gatsby-source-filesystem right now use ^9.6.0 - https://github.com/sindresorhus/got/blob/v9.6.0/readme.md (there are no notes about retry not being used in stream variant for that version).

There is always option to upgrade got (if we can that is, would need some digging to know that)

[ThyNameIsMud]

@pieh Though undocumented, looking into the relevant code the retries option is overwritten to () => 0 for streams.

It also doesn't look like the retries param is targeted the correct property. Should be options.retry.retries currently, it's options.retries.

I would recommend upgrading this lib to a more current version though. Their code has substantially improved over the last few versions.

[pieh]

I would recommend upgrading this lib to a more current version though. Their code has substantially improved over the last few versions.

I'm fine with upgrading, we just can't upgrade to latest version because that requires node@>=10.19.0 and we do support node@>=10.13, but we still can upgrade to got@10 (which only requires node@>=10)

[polarathene]

Thanks for this, it worked much better for me where my builds were failing from the timeout error.

Using the change, I received two errors where failure to retrieve the images still occurred:

 ERROR 

gatsby-plugin-remote-images ERROR: failed to process https://images.unsplash.com/photo-1518020382113-a7e8fc38eac9
RequestError: getaddrinfo ENOTFOUND images.unsplash.com


 ERROR 

gatsby-plugin-remote-images ERROR: failed to process https://images.unsplash.com/photo-1478890388506-b3f028671b40
Failed to download https://images.unsplash.com/photo-1478890388506-b3f028671b40 after 3 attempts

Both of these urls are valid, but exhibited different behaviour in Chrome when visited. The first loaded the image directly, while the 2nd triggered a download without any rendering in the browser.

I did gatsby clean followed by gatsby develop again, and this time 10/24 remote images errored with ..after 3 attempts.

My internet connection can manage 10Mbps(little over 1MB/sec), and has no problem retrieving these images manually, is gatsby trying to retrieve them in parallel, failing, then retrying with the same concurrency triggering the problem again?

Unfortunately, this seems to require doing a gatsby clean to restart the process, losing any success with the prior attempt as failed results do not try to download again otherwise.

[polarathene]

I was able to successfully download all files via GATSBY_CONCURRENT_DOWNLOAD=2 env var, which was brought to my attention with a related PR here that adds some more env vars to override defaults like the timeout limit.

If possible, it might be useful for retry behaviour to reduce the amount of concurrent requests permitted, perhaps by half? I personally did not notice a slower download time, it may have been faster for me with fewer connections(presumably because I could easily saturate the link bandwidth, but was somehow more stable with fewer requests? My connection is over wifi G or N)

[ThyNameIsMud]

@polarathene ENOTFOUND is related to DNS on the machine that is attempting to do the lookup. eg, you've defined the host in your hosts file but the machine can't read it or the nameserver isn't aware of the address. That is thrown from the got dependency. Those are the type of issues you DO want it to throw on and should be dependant on the application itself on how to handle.

The after 3 tries error happens when the asset hitsSTALL_RETRY_LIMIT. It's what happen if the service you're connecting to throws a 400/500 status code, the asset fails to finish downloading or fails to write the file. Again, these are errors you do want to throw. FWIW, the service you're connecting to may either have a rate limit or it's unable to handle the number of requests which is why dropping concurrency may help.

I do agree that making those vars overridable is worth doing, however, extending the timeouts only moves the goalpost instead of fixing the core issue.

@pieh Is it possible to push this fix through until an upgrade of got is fully vetted and approved? I've had to patch this script for our builds (to great success), but patching node modules always feels yucky.

[pieh]

Let's get this in.

Thanks for your patience.

[gatsbot]

Holy buckets, @ThyNameIsMud — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!