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

Apollo Gateway: Add make-fetch-happen fetcher as the default fetcher for the downstream services #192

Closed
Bhoomikapanwar opened this issue Sep 29, 2020 · 2 comments · Fixed by #1284

Comments

@Bhoomikapanwar
Copy link
Contributor

Currently the RemoteGraphQlDataSources uses node-fetch as the default fetcher, it will be good if we switch this to make-fetch-happen fetcher which provides us with additional features and provides support for default request options which is very much required in the real world use cases.

@Bhoomikapanwar
Copy link
Contributor Author

This will also help us to make the gateway's fetcher compatible (which uses make-fetch-happen)and will enable us to pass the gateway's fetcher to remoteGraphQLDataSource which will ultimately help us to fix this issue as well https://github.com/apollographql/apollo-server/issues/4323

@abernix abernix transferred this issue from apollographql/apollo-server Sep 30, 2020
@abernix
Copy link
Member

abernix commented Sep 30, 2020

I'll just re-quote my comments from #193 (comment), as I think they are relevant to this proposed change:

I think using make-fetch-happen as a fetcher to adjust timeouts is great, and that #188 will allow the desired configuration in that regard. Just as a point of clarity around retries functionality though: generally speaking, retries are something that needs to be handled carefully.

Typically, retries do not happen automatically on POST requests, and make-fetch-happen enforces that expectation — as noted in their documentation:

An object that can be used to tune request retry settings. Retries will only be attempted on the following conditions:

  • Request method is NOT POST AND
  • Request status is one of: 408, 420, 429, or any status in the 500-range. OR
  • Request errored with ECONNRESET, ECONNREFUSED, EADDRINUSE, ETIMEDOUT, or the fetch error request-timeout.

The following are worth noting as explicitly not retried:

  • getaddrinfo ENOTFOUND and will be assumed to be either an unreachable domain or the user will be assumed offline. If a response is cached, it will be returned immediately.
  • ECONNRESET currently has no support for restarting. It will eventually be supported but requires a bit more juggling due to streaming.

That's to say, putting make-fetch-happen in as the default fetcher will result in a lot of its benefits (including proxy support, sensible defaults, timeout configurability, request pooling, transparent gzip and deflate support, and more!), it will not result in gaining automatic retries support since every GraphQL request that we send to upstream services is sent as a, per the specification above, not retried.

While it's plausible to change the dynamics of this for GraphQL, I think we should be very cautious when contemplating whether or not to do so. For example, while a lot of GraphQL requests are often (read) querys sent as POST requests, a mutation has serious effects if retried.

If we do make make-fetch-happen the default fetcher, I would claim that we should very explicitly set retry: false on its options to make sure that if we do start sending GET requests (e.g., APQ?) at this layer, they won't inadvertently be retried in a potentially dangerous way prior to us ensuring the correct safe-guards are in place.

clenfest added a commit that referenced this issue Dec 9, 2021
As the subject says. Couple of things that I'm not sure about.

1. When I make a call to `.defaults()` do I need to pass any extra parameters? Didn't look like the `node-fetch` version did, but I want to make sure we don't want to add any extra headers or anything. For reasons stated in #192, I'm being careful and turning off retries completely, but I believe this was also the case in `node-fetch`

2. Obviously this needs to get into the CHANGELOG, but I'm assuming we do that when we cut a new version?
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 a pull request may close this issue.

2 participants