App router deduping not working as expected

[richie-south]

Verify canary release

• I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.3.0: Mon Jan 30 20:42:11 PST 2023; root:xnu-8792.81.3~2/RELEASE_X86_64
    Binaries:
      Node: 16.13.0
      npm: 9.6.4
      Yarn: N/A
      pnpm: N/A
    Relevant packages:
      next: 13.4.7
      eslint-config-next: 13.4.7
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.6

Which area(s) of Next.js are affected? (leave empty if unsure)

No response

Link to the code that reproduces this issue or a replay of the bug

https://github.com/richie-south/nextjs-cache

To Reproduce

Clone the provided repo, run commands, check terminal logs for both api and next.

1. start api server npm run api
2. start next npm run dev

Describe the Bug

Next will make multiple fetch requests to the same path on page reload even if docs says they should be deduped.

This happens on dev and production deployment.
if i reload the root page / i can see in my api logs that it will makes 3 request to my api, having fetch in my RootLayout and generateMetadata.

If i make a reload on other pages, ex /todo in my provided example, i will get 5 api requests for the same path.

Expected Behavior

I expect the requests to be deduped as illustrated in the docs. So instead of up to 5 requests it should be 1 when no cache is available.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

Other platform

_NEXT-1407

[zwarunek]

Any update/quick fixes for this issue? I am experiencing a very similar thing

[richie-south]

This is still happening on 13.4.16 btw, and is causing large performance and costs for people not aware it is happening.

@zwarunek We got around this issue by manually wrapping every fetch with the cache function documentation.

[franzwarning]

Isn't this a massive issue? I'm kinda confused how everyone is experiencing this and nobody seems to be talking about the fact that deduping doesn't really work...

[richie-south]

Still happening on 13.5.3

[raufdean]

Yes a bit annoying. We're caching the promises from the fetch in a map keyed by url and purging regularly.. Luckily not a huge throughput, so not too much of an issue.

[rjsdnql123]

Has this been resolved? I tried to reproduce the issue, but it appears that the server request is only occurring once in reality.

[richie-south]

Has this been resolved? I tried to reproduce the issue, but it appears that the server request is only occurring once in reality.

It is not fixed. You can check my example and view the server logs. There you will see multiple api calls to the server

[rjsdnql123]

Hello, @richie-south c. May i handle this issue? Please assign to me. I'll create PR.

[richie-south]

@rjsdnql123 The issue is open for anyone to fix if they have the time :)

[injae-kim]

next.js/packages/next/src/server/lib/patch-fetch.ts

Lines 501 to 504 in 218d070

	if (cacheKey && staticGenerationStore.incrementalCache) {
	handleUnlock = await staticGenerationStore.incrementalCache.lock(
	cacheKey
	)

Hi @ztanner ! (author of incremental-cache at #53030) I'm trying to solve this issue with @rjsdnql123 .
We have question about lock() on incremental-cache!

On patch-fetch.ts#L502, request try to lock incremental-cache first.
When incremental-cache is empty and multiple requests with same url(cacheKey) comes in, these requests can get lock incremental-cache at the same time?

next.js/packages/next/src/server/lib/incremental-cache/index.ts

Lines 241 to 252 in 218d070

	const existingLock = this.locks.get(cacheKey)
	if (existingLock) {
	await existingLock
	} else {
	const newLock = new Promise<void>((resolve) => {
	unlockNext = async () => {
	resolve()
	}
	})
	this.locks.set(cacheKey, newLock)

On above code, I think createLockIfAbsent logic is not synchronized over multiple requests so maybe multiple requests can create lock at the same time? 🤔