Skip to content

Commit

Permalink
[dynamicIO] Avoid triggering memory leak false positive with makeHang…
Browse files Browse the repository at this point in the history
…ingPromise (#71576)

makeHangingPromise returns a promise that rejects when an AbortSignal
aborts. If too many are created however Node warns about a possible
memory leak. We shouldn't increase the max count becuase this may
prevent detection of legitmate memory leaks in user code. As an
alternative we delegate aborts through a weakmap and attach only a
single listener. This is trivially slower and more memory but safer
semantics. This code only runs during revalidate and build so it does
not have a perf impact on production streaming use cases.
  • Loading branch information
gnoff authored Oct 21, 2024
1 parent aa474ee commit 73bbc69
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 3 deletions.
33 changes: 30 additions & 3 deletions packages/next/src/server/dynamic-rendering-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,44 @@ export function makeHangingPromise<T>(
expression: string
): Promise<T> {
const hangingPromise = new Promise<T>((_, reject) => {
signal.addEventListener('abort', () => {
function abort() {
reject(
new Error(
`During prerendering, ${expression} rejects when the prerender is complete. Typically these errors are handled by React but if you move ${expression} to a different context by using \`setTimeout\`, \`unstable_after\`, or similar functions you may observe this error and you should handle it in that context.`
)
)
})
}

if (signal.aborted) {
abort()
return
}

let listeners = listenersForSignal.get(signal)
if (listeners) {
listeners.push(abort)
} else {
listeners = [abort]
listenersForSignal.set(signal, listeners)
signal.addEventListener('abort', onAbort.bind(null, listeners), {
once: true,
})
}
})
// We are fine if no one actually awaits this promise. We shouldn't consider this an unhandled rejection so
// we attach a noop catch handler here to suppress this warning. If you actually await somewhere or construct
// your own promise out of it you'll need to ensure you handle the error when it rejects.
hangingPromise.catch(() => {})
hangingPromise.catch(ignoreReject)
return hangingPromise
}

function ignoreReject() {}

function onAbort(listeners: Array<AbortListener>) {
for (let i = 0; i < listeners.length; i++) {
listeners[i]()
}
}

type AbortListener = () => void
const listenersForSignal = new WeakMap<AbortSignal, Array<AbortListener>>()
6 changes: 6 additions & 0 deletions test/e2e/app-dir/dynamic-io/dynamic-io.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ describe('dynamic-io', () => {
expect(next.cliOutput).not.toMatch('Error occurred prerendering page')
})

if (!isNextDev) {
it('should not warn about potential memory leak for even listeners on AbortSignal', async () => {
expect(next.cliOutput).not.toMatch('MaxListenersExceededWarning')
})
}

it('should prerender fully static pages', async () => {
let $ = await next.render$('/cases/static', {})
if (isNextDev) {
Expand Down

0 comments on commit 73bbc69

Please sign in to comment.