perf: parallelize jsDelivr README fallback probes#2384
perf: parallelize jsDelivr README fallback probes#2384trivikr wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates server/utils/readme-loaders.ts to fetch jsDelivr README candidates in concurrent batches of three (via Promise.all) using a new constant JSDELIVR_README_FETCH_BATCH_SIZE. fetchReadmeFromJsdelivr now returns the first non-empty response from each batch (fetch errors or non-OK responses map to null). A new helper buildReadmeFetchCandidates excludes a provided readmeFilename from standard candidates. resolvePackageReadmeSource is changed to try the explicit readmeFilename first and, if that yields null, fall back to fetching the batched candidate list. Tests were added/expanded to verify batching, selective response.text() invocation, and the new candidate ordering; exported function signatures remain unchanged. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Also updated PR to fetch provided readme before starting the fallback batch in 72bb0b2 |
trivikr
force-pushed
the
fetch-readme-parallel
branch
from
36fced5 to
4e00761
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/utils/readme-loaders.ts (1)
115-125: Fix typoed inline commentsThere are a couple of typos (
unsucessful,sometihng) in the inline comments. Quick polish for readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 248c6759-6520-45f8-8aca-2e08376a991a
📒 Files selected for processing (2)
server/utils/readme-loaders.tstest/unit/server/utils/readme-loaders.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/server/utils/readme-loaders.spec.ts
| const responses = await Promise.all( | ||
| batch.map(async filename => { | ||
| try { | ||
| const url = `https://cdn.jsdelivr.net/npm/${packageName}${versionSuffix}/${filename}` | ||
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| return null | ||
| } | ||
|
|
||
| return response | ||
| } catch { | ||
| return null | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| for (const response of responses) { | ||
| const text = await response?.text() | ||
| if (text?.trim()) { | ||
| return text | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js fetch (undici), should unread Response bodies be consumed or cancelled to promptly release underlying connections?
💡 Result:
Yes—don’t leave a Node.js/undici fetch() response body unread if you care about promptly releasing resources.
- In Node (undici), relying on GC to eventually clean up an unconsumed body is not deterministic and can cause excessive connection usage, reduced keep‑alive reuse, and even stalls/deadlocks when you run out of connections. You should always either consume or cancel the response body. [1]
- If you only needed headers, prefer
method: 'HEAD'so there’s no body to drain/cancel. [1] - Node’s own docs/examples also call out that consuming the response body is mandatory in this context. [2]
Practical rule
- If you want connection reuse: drain/consume the body (e.g.,
await res.arrayBuffer(),await res.text(), or read the stream to EOF). [1] - If you want to stop early: cancel the body (
await res.body?.cancel()), which promptly frees the socket from being “owned” by that body stream (typically by closing that connection rather than reusing it). [1][3]
Example (don’t care about body):
const res = await fetch(url);
await res.body?.cancel(); // release promptly (no keep-alive reuse)Sources: [1] [2] [3]
🏁 Script executed:
# First, let's locate and read the readme-loaders.ts file
find . -name "readme-loaders.ts" -type fRepository: npmx-dev/npmx.dev
Length of output: 93
🏁 Script executed:
# Read the file to verify the code structure and lines around 47-67
cat -n server/utils/readme-loaders.ts | head -80Repository: npmx-dev/npmx.dev
Length of output: 3034
Cancel unread batch responses before early return to avoid connection resource leaks
When a matching README is found, the loop returns immediately without consuming the remaining responses in the batch. In Node.js (undici), unread fetch() response bodies retain their underlying connections indefinitely until garbage collected, which is not deterministic. This can cause excessive connection usage and potential connection pool exhaustion in long-running runtimes.
Suggested fix
for (const response of responses) {
const text = await response?.text()
if (text?.trim()) {
+ await Promise.all(
+ responses.map(other =>
+ other && other !== response && !other.bodyUsed
+ ? other.body?.cancel().catch(() => undefined)
+ : undefined,
+ ),
+ )
return text
}
}
+
+ await Promise.all(
+ responses.map(response =>
+ response && !response.bodyUsed
+ ? response.body?.cancel().catch(() => undefined)
+ : undefined,
+ ),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const responses = await Promise.all( | |
| batch.map(async filename => { | |
| try { | |
| const url = `https://cdn.jsdelivr.net/npm/${packageName}${versionSuffix}/${filename}` | |
| const response = await fetch(url) | |
| if (!response.ok) { | |
| return null | |
| } | |
| return response | |
| } catch { | |
| return null | |
| } | |
| }), | |
| ) | |
| for (const response of responses) { | |
| const text = await response?.text() | |
| if (text?.trim()) { | |
| return text | |
| } | |
| const responses = await Promise.all( | |
| batch.map(async filename => { | |
| try { | |
| const url = `https://cdn.jsdelivr.net/npm/${packageName}${versionSuffix}/${filename}` | |
| const response = await fetch(url) | |
| if (!response.ok) { | |
| return null | |
| } | |
| return response | |
| } catch { | |
| return null | |
| } | |
| }), | |
| ) | |
| for (const response of responses) { | |
| const text = await response?.text() | |
| if (text?.trim()) { | |
| await Promise.all( | |
| responses.map(other => | |
| other && other !== response && !other.bodyUsed | |
| ? other.body?.cancel().catch(() => undefined) | |
| : undefined, | |
| ), | |
| ) | |
| return text | |
| } | |
| } | |
| await Promise.all( | |
| responses.map(response => | |
| response && !response.bodyUsed | |
| ? response.body?.cancel().catch(() => undefined) | |
| : undefined, | |
| ), | |
| ) |
3 participants
🔗 Linked issue
N/A
🧭 Context
README fallback probing in
server/utils/readme-loaders.tswas strictly sequential. On cache misses, packages with missing or nonstandard README filenames could pay for several failed jsDelivr requests before the loader found the right file or gave up, which made worst-case README render time noticeably slower than necessary.📚 Description
This changes the jsDelivr README fallback path to reduce that long-tail latency while keeping the fallback behavior conservative.
readmeFilenamefirst when it exists.test/unit/server/utils/readme-loaders.spec.ts.