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.

Sign up for GitHub

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

fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution #2490

Merged
merged 12 commits into from
Jun 26, 2024
+148 −32
Merged

fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution #2490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c6a8699
fix: track revalidate / cdn purge to ensure it finishes execution and…
pieh Jun 14, 2024
bd9771b
test: don't destroy lambda env vars until response stream finished
pieh Jun 17, 2024
55c7843
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 18, 2024
e88dd8c
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 18, 2024
b475810
fix: remove 'any' types from next's response proxy
pieh Jun 24, 2024
5c1eddb
Merge remote-tracking branch 'origin/main' into fix/track-background-…
pieh Jun 24, 2024
08f666d
Merge remote-tracking branch 'origin/main' into fix/track-background-…
pieh Jun 24, 2024
dd43ce8
test: add a case for not awaited res.revalidate
pieh Jun 24, 2024
a72a0cd
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 24, 2024
17d9aec
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 24, 2024
2a054cc
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 25, 2024
1e2f02b
Merge branch 'main' into fix/track-background-revalidation
pieh Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/run/handlers/cache.cts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,19 +244,33 @@ export class NetlifyCacheHandler implements CacheHandler {
if (requestContext?.didPagesRouterOnDemandRevalidate) {
const tag = `_N_T_${key === '/index' ? '/' : key}`
getLogger().debug(`Purging CDN cache for: [${tag}]`)
purgeCache({ tags: [tag] }).catch((error) => {
// TODO: add reporting here
getLogger()
.withError(error)
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
})
requestContext.trackBackgroundWork(
purgeCache({ tags: [tag] }).catch((error) => {
// TODO: add reporting here
getLogger()
.withError(error)
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
}),
)
Comment on lines +329 to +336
Copy link
Contributor Author

Choose a reason for hiding this comment

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

making sure we track purgeCache part of the pages router on-demand revalidation so it completes before function execution gets suspended

}
}
})
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
async revalidateTag(tagOrTags: string | string[], ...args: any) {
const revalidateTagPromise = this.doRevalidateTag(tagOrTags, ...args)

const requestContext = getRequestContext()
if (requestContext) {
requestContext.trackBackgroundWork(revalidateTagPromise)
}

return revalidateTagPromise
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
private async doRevalidateTag(tagOrTags: string | string[], ...args: any) {
Comment on lines 343 to +355
Copy link
Contributor Author

Choose a reason for hiding this comment

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

App Router revalidateTag and revalidatePath don't return a promise so user can't even await on the revalidation work in their code. Instead we will track it internally (should not affect timing of the response, just that we should keep execution running and not suspend before this is completed)

getLogger().withFields({ tagOrTags, args }).debug('NetlifyCacheHandler.revalidateTag')

const tags = Array.isArray(tagOrTags) ? tagOrTags : [tagOrTags]
Expand All @@ -275,7 +289,7 @@ export class NetlifyCacheHandler implements CacheHandler {
}),
)

purgeCache({ tags }).catch((error) => {
await purgeCache({ tags }).catch((error) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't change much, because .revalidateTag overall was not awaited on and all of this is ~background work that won't block response

// TODO: add reporting here
getLogger()
.withError(error)
Expand Down
9 changes: 8 additions & 1 deletion src/run/revalidate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { ServerResponse } from 'node:http'
import { isPromise } from 'node:util/types'
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL


import type { RequestContext } from './handlers/request-context.cjs'

Expand All @@ -12,7 +13,13 @@ export const nextResponseProxy = (res: ServerResponse, requestContext: RequestCo
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return async function newRevalidate(...args: any[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why we're using all these dangerous anys here? We should be able to reference the type of the original function here instead, shouldn't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why, but at least with code as is we ourselves don't use args directly and only forward them to actual res.revalidate method so in this case (at least this specific line, as there are some other any in this module) we don't necessarily care about types, but there is no reason not to add proper types here I think. I'll take a look and see if I can get rid of all those any in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b475810 got rid of any usage. Added type guard there is pretty rudimentary, but I didn't really see a way to make it better/narrower (it only check key - which we already were doing + started checking if value is at least a function)

requestContext.didPagesRouterOnDemandRevalidate = true
return originalValue?.apply(target, args)

const result = originalValue?.apply(target, args)
if (result && isPromise(result)) {
requestContext.trackBackgroundWork(result)
}

return result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

User's CAN await pages router res.revalidate(), so if user is doing that, this is not changing anything, but it's quite easy to miss await in user code (especially that the promise would resolve to void anyway, so user would not be doing anything with res.revalidate() return value (other than hopefully await it).

This just make sure that if user doesn't await it, we will (not blocking a response, just tracking this as background work)

}
}
return originalValue
Expand Down
5 changes: 4 additions & 1 deletion tests/fixtures/page-router/pages/api/revalidate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
export default async function handler(req, res) {
try {
await res.revalidate('/static/revalidate-manual')
// res.revalidate returns a promise that can be awaited to wait for the revalidation to complete
// if user doesn't await it, we still want to ensure the revalidation is completed, so we internally track
// this as "background work" to ensure it completes before function suspends execution
res.revalidate('/static/revalidate-manual')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might make our page router on-demand revalidate tests flaky, as we do use to trigger on-demand revalidation, then sleep for a bit and then check revalidated page, but because we don't await here, the response from revalidate api endpoint would happen before revalidation completed (at least Next.js side of it)

on the other hand it would be good to verify that background tracking actually works so I would prefer to have this and potentially increase sleep timeouts in tests (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be better to explicitly not await this call in a specific fixture/test for this behavior, and await it in all other tests. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to have 2 cases here so in case those e2e tests fail it's easier to tell wether there is overall problem with revalidation or specifically with not awaited revalidation. Adding variants here would be much smoother after #2495 is in as it does add ~test.each kind of setup to on-demand revalidation tests where it's super easy to add another case, so I'll get this one in first and then revert this change and instead add another API endpoint that doesn't await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dd43ce8 added separate case for not awaiting res.revalidate on top of existing ones

return res.json({ code: 200, message: 'success' })
} catch (err) {
return res.status(500).send({ code: 500, message: err.message })
Expand Down
Loading