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

Allow supplying a retry handler to generated API clients #271

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

augustuswm
Copy link
Contributor

@augustuswm augustuswm commented Sep 20, 2024

This could be something implemented outside of a client, but it would be nice to have some kind of retry facility within the client itself.

This stemmed from an issue encountered trying to integrate a generated client with the remix-auth-oauth2 package on Node 19+ over http. Node 19 introduced a change to the default http agent which set the keepAlive setting to true, meaning that connections will be re-used by default compared to Node 18.

In the case of our custom OAuth2 strategy, remix-auth-oauth2 makes its own request to the token endpoint with the Connection: Close header, after which we immediately make a request to the same server via the generated client. Since the agent has been configured to re-use connection, we seem to be reusing the socket that has yet to been closed on the client side resulting in the server issuing a reset since it has closed the connection. The generated client does not handle errors from the initial fetch request that is made, so this bubbles up to the application.

This PR introduces an optional retry handler that wraps both fetch and response errors, allowing API clients to be constructed with customized retry logic based on their needs.

An example of how this may be used in this use case is:

// Retry a single time when receiving an ECONNRESET
const retryFactory = () => {
  const limit = 1;
  let retries = 0;
  return (err: any) => {
    if (retries < limit && err.type === 'system' && err.errno === 'ECONNRESET' && err.code === 'ECONNRESET') {
      retries += 1
      return true
    } else {
      return false
    }
}

@augustuswm
Copy link
Contributor Author

I think the any type here is correct given my understanding of throw, is there a better type that should be used?

retries += 1
return true
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will review in more detail, but my immediate reaction is that as implemented, the retry logic is still entirely outside the client and fetchWithRetry doesn't add that much. I'll try and put together a version of this that doesn't require fetchWithRetry and we can compare.

Here's an example in the console that uses the p-retry library. It's not exactly analogous, but the mutateAsync call is very close to calling the client method directly.

https://github.com/oxidecomputer/console/blob/1625d02a540035e6b10e6222e7ef709691dda7f1/app/forms/image-upload.tsx#L424-L429

Copy link
Contributor Author

@augustuswm augustuswm Sep 20, 2024

Choose a reason for hiding this comment

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

This is super low priority, I'm doing something similar already where I am wrapping the client in a retry. It is certainly acceptable that we don't want this as part of the client itself. I think the only reason for it, is that it is likely almost entirely incorrect to use a generated client without a retry around it given that we are using the default node http(s) agents.

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