Skip to content

perf: Cache HTTP requests across all pages #174

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

Merged
azu merged 2 commits into from
Feb 2, 2026
Merged

perf: Cache HTTP requests across all pages #174

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1b68fa
perf: Cache HTTP requests across all pages
ajfAfg Jan 21, 2026
31aae9c
perf: Always consume `Response.body`
ajfAfg Jan 21, 2026
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
35 changes: 30 additions & 5 deletions src/no-dead-link.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,12 @@ const createCheckAliveURL = (ruleOptions: Options, resolvePath: (path: string, b
// and see what kind of redirect is occurring
redirect: "manual" as RequestRedirect
};

// Declare the variable outside the try-catch block to ensure the body is always consumed in `finally`.
let res: Response | null = null;

try {
const res = await fetchWithDefaults(uri, opts);
res = await fetchWithDefaults(uri, opts);
const errorResult = {
ok: false,
redirected: true,
Expand Down Expand Up @@ -214,6 +218,7 @@ const createCheckAliveURL = (ruleOptions: Options, resolvePath: (path: string, b
const finalRes = await fetchWithDefaults(redirectedUrl, { ...opts, redirect: "follow" });
const url = URL.parse(uri);
const hash = url?.hash || null;
await finalRes.body?.cancel();
return {
ok: finalRes.ok,
redirected: true,
Expand Down Expand Up @@ -264,6 +269,10 @@ const createCheckAliveURL = (ruleOptions: Options, resolvePath: (path: string, b
ok: false,
message: ex.message
};
} finally {
if (res && !res.body?.locked) {
await res.body?.cancel();
}
}
Comment on lines 187 to 276
Copy link
Member

@azu azu Jan 22, 2026

Choose a reason for hiding this comment

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

Is it okay to understand that this change has nothing to do with Cache?

https://github.com/nodejs/undici?tab=readme-ov-file#garbage-collection
I feel like it's good to explicitly consume because you can't rely on GC with Node fetch

Copy link
Contributor Author

@ajfAfg ajfAfg Jan 22, 2026

Choose a reason for hiding this comment

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

No, in my environment, after placing the cache globally, it started hanging occasionally. Therefore, I believe it is appropriate to include this change in this PR.

};
};
Expand All @@ -289,6 +298,11 @@ async function isAliveLocalFile(filePath: string): Promise<AliveFunctionReturn>
}
}

const memorizedIsAliveURIByOptions = new Map<
string,
ReturnType<typeof pMemoize<ReturnType<typeof createCheckAliveURL>>>
>();
Copy link
Member

@azu azu Jan 22, 2026

Choose a reason for hiding this comment

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

If you have a Cache Map globally, you will behave like a memory leak if you don't condition it by size or TTL like LRU Cache.
CLI is not a big problem, but if you run it in a server-like shape or editor, it will always refer to the same global cache, so I think memory consumption will increase linearly

Copy link
Contributor Author

@ajfAfg ajfAfg Jan 22, 2026

Choose a reason for hiding this comment

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

The function pMemoize allows setting the cache TTL. Since the rule textlint-rule-no-dead-link sets it to 30 seconds by default, I believe there is no need to implement TTL again. What do you think?

Copy link
Member

@azu azu Feb 2, 2026

Choose a reason for hiding this comment

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

📝 memorizedIsAliveURIByOptions is still global variables.


const reporter: TextlintRuleReporter<Options> = (context, options) => {
const { Syntax, getSource, report, RuleError, fixer, getFilePath, locator } = context;
const helper = new RuleHelper(context);
Expand All @@ -307,10 +321,21 @@ const reporter: TextlintRuleReporter<Options> = (context, options) => {
}
}
};
const isAliveURI = createCheckAliveURL(ruleOptions, resolvePath);
const memorizedIsAliveURI = pMemoize(isAliveURI, {
maxAge: ruleOptions.linkMaxAge
});
const memorizedIsAliveURI = (() => {
const memoOptionsKey = JSON.stringify(ruleOptions);
const func = memorizedIsAliveURIByOptions.get(memoOptionsKey);
if (!func) {
const isAliveURI = createCheckAliveURL(ruleOptions, resolvePath);
const func = pMemoize(isAliveURI, {
maxAge: ruleOptions.linkMaxAge
});
memorizedIsAliveURIByOptions.set(memoOptionsKey, func);
return func;
}

return func;
})();

/**
* Checks a given URI's availability and report if it is dead.
* @param {TextLintNode} node TextLintNode the URI belongs to.
Expand Down