-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
[feature] custom retry callback #332
Changes from 1 commit
12969ad
eb55a0b
ec0c280
bf2c6e1
b9fdd6d
8e8dc98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ const fastifyReplyFrom = fp(function from (fastify, opts, next) { | |
]) | ||
|
||
const retryMethods = new Set(opts.retryMethods || [ | ||
'GET', 'HEAD', 'OPTIONS', 'TRACE' | ||
'GET', 'HEAD', 'OPTIONS', 'TRACE', 'POST', 'PATCH' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
]) | ||
|
||
const cache = opts.disableCache ? undefined : lru(opts.cacheURLs || 100) | ||
|
@@ -60,6 +60,7 @@ const fastifyReplyFrom = fp(function from (fastify, opts, next) { | |
const onError = opts.onError || onErrorDefault | ||
const retriesCount = opts.retriesCount || 0 | ||
const maxRetriesOn503 = opts.maxRetriesOn503 || 10 | ||
const customRetry = opts.customRetry || undefined | ||
|
||
if (!source) { | ||
source = req.url | ||
|
@@ -143,7 +144,7 @@ const fastifyReplyFrom = fp(function from (fastify, opts, next) { | |
const contentLength = requestHeaders['content-length'] | ||
let requestImpl | ||
if (retryMethods.has(method) && !contentLength) { | ||
requestImpl = createRequestRetry(request, this, retriesCount, retryOnError, maxRetriesOn503) | ||
requestImpl = createRequestRetry(request, this, retriesCount, retryOnError, maxRetriesOn503, customRetry) | ||
} else { | ||
requestImpl = request | ||
} | ||
|
@@ -251,29 +252,51 @@ function isFastifyMultipartRegistered (fastify) { | |
return (fastify.hasContentTypeParser('multipart') || fastify.hasContentTypeParser('multipart/form-data')) && fastify.hasRequestDecorator('multipart') | ||
} | ||
|
||
function createRequestRetry (requestImpl, reply, retriesCount, retryOnError, maxRetriesOn503) { | ||
function createRequestRetry (requestImpl, reply, retriesCount, retryOnError, maxRetriesOn503, customRetry) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
function requestRetry (req, cb) { | ||
let retries = 0 | ||
|
||
function run () { | ||
requestImpl(req, function (err, res) { | ||
// Magic number, so why not 42? We might want to make this configurable. | ||
let retryAfter = 42 * Math.random() * (retries + 1) | ||
|
||
if (res && res.headers['retry-after']) { | ||
retryAfter = res.headers['retry-after'] | ||
const defaultRetryAfter = () => { | ||
let retryAfter = 42 * Math.random() * (retries + 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
if (res && res.headers['retry-after']) { | ||
retryAfter = res.headers['retry-after'] | ||
} | ||
return retryAfter | ||
} | ||
if (!reply.sent) { | ||
// always retry on 503 errors | ||
|
||
const defaultRetry = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (res && res.statusCode === 503 && req.method === 'GET') { | ||
if (retriesCount === 0 && retries < maxRetriesOn503) { | ||
// we should stop at some point | ||
return retry(retryAfter) | ||
return true | ||
} | ||
} else if (retriesCount > retries && err && err.code === retryOnError) { | ||
return retry(retryAfter) | ||
return true | ||
} | ||
return false | ||
} | ||
|
||
if (!reply.sent) { | ||
if (customRetry && customRetry.handler) { | ||
const retryAfter = customRetry.handler(req, res, defaultRetryAfter, defaultRetry) | ||
if (retryAfter) { | ||
const customRetries = customRetry.retries || 1 | ||
if (++retries < customRetries) { | ||
return retry(retryAfter) | ||
} | ||
} | ||
} else { | ||
if (defaultRetry()) { | ||
return retry(defaultRetryAfter()) | ||
} | ||
} | ||
} | ||
|
||
cb(err, res) | ||
}) | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,114 @@ | ||||||
'use strict' | ||||||
|
||||||
const { test } = require('tap') | ||||||
const Fastify = require('fastify') | ||||||
const From = require('..') | ||||||
const http = require('node:http') | ||||||
const got = require('got') | ||||||
|
||||||
function serverWithCustomError (stopAfter, statusCodeToFailOn) { | ||||||
let requestCount = 0 | ||||||
return http.createServer((req, res) => { | ||||||
if (requestCount++ < stopAfter) { | ||||||
res.statusCode = statusCodeToFailOn | ||||||
res.setHeader('Content-Type', 'text/plain') | ||||||
return res.end('This Service is Unavailable') | ||||||
} else { | ||||||
res.statusCode = 205 | ||||||
res.setHeader('Content-Type', 'text/plain') | ||||||
return res.end(`Hello World ${requestCount}!`) | ||||||
} | ||||||
}) | ||||||
} | ||||||
|
||||||
async function setupServer (t, fromOptions = {}, statusCodeToFailOn = 500, stopAfter = 4) { | ||||||
const target = serverWithCustomError(stopAfter, statusCodeToFailOn) | ||||||
await target.listen({ port: 0 }) | ||||||
t.teardown(target.close.bind(target)) | ||||||
|
||||||
const instance = Fastify() | ||||||
instance.register(From, { | ||||||
base: `http://localhost:${target.address().port}` | ||||||
}) | ||||||
|
||||||
instance.get('/', (request, reply) => { | ||||||
reply.from(`http://localhost:${target.address().port}`, fromOptions) | ||||||
}) | ||||||
|
||||||
t.teardown(instance.close.bind(instance)) | ||||||
await instance.listen({ port: 0 }) | ||||||
|
||||||
return { | ||||||
instance | ||||||
} | ||||||
} | ||||||
|
||||||
test('a 500 status code with no custom handler should fail', async (t) => { | ||||||
const { instance } = await setupServer(t) | ||||||
|
||||||
try { | ||||||
await got.get(`http://localhost:${instance.server.address().port}`, { retry: 0 }) | ||||||
} catch (error) { | ||||||
t.ok(error instanceof got.RequestError, 'should throw RequestError') | ||||||
t.end() | ||||||
} | ||||||
}) | ||||||
|
||||||
test("a server 500's with a custom handler and should revive", async (t) => { | ||||||
const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => { | ||||||
if (res && res.statusCode === 500 && req.method === 'GET') { | ||||||
return 300 | ||||||
} | ||||||
return null | ||||||
} | ||||||
|
||||||
const { instance } = await setupServer(t, { customRetry: { handler: customRetryLogic, retries: 10 } }) | ||||||
|
||||||
const res = await got.get(`http://localhost:${instance.server.address().port}`, { retry: 0 }) | ||||||
|
||||||
t.equal(res.headers['content-type'], 'text/plain') | ||||||
t.equal(res.statusCode, 205) | ||||||
t.equal(res.body.toString(), 'Hello World 5!') | ||||||
}) | ||||||
|
||||||
test("a server 503's with a custom handler for 500 but the custom handler never registers the default so should fail", async (t) => { | ||||||
// the key here is our customRetryHandler doesn't register the deefault handler and as a result it doesn't work | ||||||
const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => { | ||||||
if (res && res.statusCode === 500 && req.method === 'GET') { | ||||||
return 300 | ||||||
} | ||||||
return null | ||||||
} | ||||||
|
||||||
const { instance } = await setupServer(t, { customRetry: { handler: customRetryLogic, retries: 10 } }, 503) | ||||||
|
||||||
try { | ||||||
await got.get(`http://localhost:${instance.server.address().port}`, { retry: 0 }) | ||||||
} catch (error) { | ||||||
t.equal(error.message, 'Response code 503 (Service Unavailable)') | ||||||
t.end() | ||||||
} | ||||||
}) | ||||||
|
||||||
test("a server 503's with a custom handler for 500 and the custom handler registers the default so it passes", async (t) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are down with the language change
Suggested change
|
||||||
const customRetryLogic = (req, res, registerDefaultRetry, defaultRetryAfter) => { | ||||||
// registering the default retry logic for non 500 errors if it occurs | ||||||
if (registerDefaultRetry()) { | ||||||
return defaultRetryAfter | ||||||
} | ||||||
|
||||||
if (res && res.statusCode === 500 && req.method === 'GET') { | ||||||
return 300 | ||||||
} | ||||||
|
||||||
return null | ||||||
} | ||||||
|
||||||
const { instance } = await setupServer(t, { customRetry: { handler: customRetryLogic, retries: 10 } }, 503) | ||||||
|
||||||
const res = await got.get(`http://localhost:${instance.server.address().port}`, { retry: 0 }) | ||||||
|
||||||
t.equal(res.headers['content-type'], 'text/plain') | ||||||
t.equal(res.statusCode, 205) | ||||||
t.equal(res.body.toString(), 'Hello World 6!') | ||||||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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