[feature] custom retry callback

[MikePresman]

This PR implements two fundamental changes.

Edit: 1. I have added PATCH and POST as given default retryMethods see review by airhorns
2. I have added the option to pass a callback to handle retries in a custom fashion as defined by the client.

Point 2. Described

The motivation for this change is that out of the box reply-from does not retry 500's so gracefully handling such cases is not possible. As a result with this change you are able to write your own callback to retry 500's as you wish (or any other kind of error - using 500's here as an exemplar).

This can be done quite simply.

const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => {
   if (res && res.statusCode === 500 && req.method === 'GET') {
     return 300
   }
   return null
 }

...
Fastify.reply(From, customRetry: { handler: customRetryLogic, retries: 10 }}

Note this new API is an object which has two keys. handler and retries where handler is a required key and retries is optional but strongly encouraged.

---

There is an important intricacy to this API.

If no customRetry object is passed then business as usual in the library. Retries work as they are expected for default cases.

But if a customRetry is passed then it is up to responsibility of the callback to be aware that if they expect default behaviour to co-exist they must register it. If they don't (the above example DID NOT) all default cases of errors (such as 503s) won't be retried.

Example to get default behaviour in a customRetry

const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => {
   // Registering the default retry logic so if a default case occurs we handle it.
   if (registerDefaultRetry()){
     return defaultRetryAfter; // Some new magic is that we can override our headers and pass a new retry-after value.
   }

   if (res && res.statusCode === 500 && req.method === 'GET') {
     return 300
   }
   return null
 }

CC: @airhorns

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[airhorns]

I think this makes this a breaking change when it doesn't need to be. I think it'd be simpler if we just passed this in gadget side?

[airhorns]

What do you think about including the 503 retry behaviour in this function as well? To me it doesn't really seem extra special such that a custom handler may want to be a client of that logic as well.

[airhorns]

Why two separate functions for the duration and if we should retry or not? I think it'd be simpler with one function that returns null or a number, null meaning don't retry, and a number meaning retry after that many milliseconds.

[airhorns]

I think it might be clearer not to describe this as "registering" another retry handler (both in this PR and in the test code), we're not really adding a handler to an event emitter or something, but just passing an option.

Also, I think that a custom retry handler needs to be passed the number of attempts so far if it wants to calculate an exponential delay. What do you think about this API instaead

Suggested change

	const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => {
	//If this block is not included all non 500 errors will not be retried
	if (registerDefaultRetry()){
	return defaultRetryAfter;
	}
	//Custom retry logic
	if (res && res.statusCode === 500 && req.method === 'GET') {
	return 300
	}
	return null
	}
	const customRetryLogic = (req, res, attemptCount, getDefaultDelay) => {
	const defaultDelay = getDefaultDelay();
	if (defaultDelay) return defaultDelay();
	if (res && res.statusCode === 500 && req.method === 'GET') {
	return 300
	}
	return null
	}

[airhorns]

The method signature to this function seems a little complicated. I think it might be simpler if the caller just built one "give me the retry delay" function, and passed it into this thing. That function can do whatever it needs to based on the options, and kind of bottles up all the logic, so this thing doesn't really need to care about the "when" of retrying, and is just about actually doing it based on the output of that function.

[airhorns]

If you are down with the language change

Suggested change

	test("a server 503's with a custom handler for 500 and the custom handler registers the default so it passes", async (t) => {
	test("custom retry delay functions can invoke the default delay", async (t) => {

[mcollina]

lgtm

[airhorns]

LGTM after comments!

[airhorns]

Can you avoid making these unrelated changes in this PR 🙏

[MikePresman]

I think I ran lint incorrectly my bad !
Will fix.