fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159
fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159hiteshjambhale wants to merge 3 commits into
Conversation
getNodeAjaxOptions cached responses only after they resolved, so concurrent callers for the same URL (e.g. every column row's Data Type dropdown mounting at once) all saw an empty cache and each fired its own identical HTTP GET. Track in-flight requests in a module-level Map keyed by the resolved URL and query params. Concurrent callers now await the same underlying request; the first to resolve populates the cache as before, and the entry is cleaned up on settle so the map does not leak. Fixes pgadmin-org#10155 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughConcurrent GET requests from ChangesAJAX request deduplication
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant getNodeAjaxOptions
participant getInflight
participant api.get
getNodeAjaxOptions->>getInflight: Request fullUrl with params
alt No matching pending request
getInflight->>api.get: Start GET request
api.get-->>getInflight: Return response
getInflight-->>getNodeAjaxOptions: Shared response promise
else Matching pending request
getInflight-->>getNodeAjaxOptions: Reuse shared promise
end
getNodeAjaxOptions-->>getNodeAjaxOptions: Cache and transform response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/browser/static/js/node_ajax.js`:
- Around line 137-145: Move the cacheNode.cache invocation out of the shared
api.get promise callback and into the per-caller request.then block before
transform(resData). Use each caller’s own otherParams, cacheNode, nodeObj, url,
treeNodeInfo, and cacheLevel values so concurrent requests apply independent
cache logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27d542f2-1ebc-4cbb-881c-b8a2f93d5bc4
📒 Files selected for processing (1)
web/pgadmin/browser/static/js/node_ajax.js
| // Share a single in-flight request among all concurrent callers asking | ||
| // for the same URL + params, so we don't fire duplicate GETs before the | ||
| // first response lands and populates the cache. | ||
| let inflightKey = fullUrl + '#' + JSON.stringify(otherParams.urlParams || {}); |
There was a problem hiding this comment.
Some observations -
- Not the best way to create a unique key
{
a:1,
b:2
}
vs
{
b:2,
a:1
}
And a nested object will make it worse.
- A better place to put such logic is inside the Axios wrapper, so that it will be used by the entire app
|
@hiteshjambhale fix is working, we only get one API call for column details instead of N(number of columns). |
Address review feedback on the getNodeAjaxOptions dedup:
- Move the in-flight request sharing out of node_ajax.js and into
api_instance.js as a reusable getInflight() helper, so any part of
the app can dedupe identical concurrent GETs, not just node options.
- Build the in-flight key with a stable, key-order-independent
stringify (sorted keys, recursive) so {a:1,b:2} and {b:2,a:1} and
nested params resolve to the same key.
- Run caching and transform per-caller in each caller's own .then on
the shared response, so concurrent callers with differing params
each cache correctly instead of inheriting the first caller's.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/static/js/api_instance.js`:
- Line 31: Update the in-flight request key generated by getInflight in
api_instance.js to include the effective Axios/API request context, not only the
URL and params. Ensure requests using different caller-specific headers,
authorization/CSRF values, or runtime options cannot share cached results, while
preserving deduplication for identical request contexts.
- Around line 56-59: Update getInflight() so each shared Axios GET request uses
a finite timeout or cancellation mechanism before being stored in
_inflightGetRequests. Ensure the map entry is removed when the request aborts,
rejects, or resolves, preventing stalled requests from retaining the key
indefinitely.
- Around line 35-44: Update stableStringify, used by getInflight, to produce
collision-free keys for supported parameter values and reject unsupported
config.params inputs, including undefined-containing arrays, Date, Map, and
cyclic structures. Preserve deterministic object-key ordering while ensuring
distinct JSON-like types cannot serialize to the same key; add coverage for
these collision and rejection cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 28f67f76-5bdb-4492-8df2-e84e32878bcb
📒 Files selected for processing (2)
web/pgadmin/browser/static/js/node_ajax.jsweb/pgadmin/static/js/api_instance.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/static/js/node_ajax.js
| request = api.get(url, config).finally(() => { | ||
| _inflightGetRequests.delete(key); | ||
| }); | ||
| _inflightGetRequests.set(key, request); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 '\b(timeout|AbortController|signal)\b' \
web/pgadmin/static/js/api_instance.js \
web/pgadmin/browser/static/js/node_ajax.jsRepository: pgadmin-org/pgadmin4
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate relevant files =="
git ls-files | fgrep -E '(^|/)api_instance\.js$|node_ajax\.js$|axios|api' | sed -n '1,200p'
echo
echo "== api_instance.js outline/size/contents =="
wc -l web/pgadmin/static/js/api_instance.js
sed -n '1,220p' web/pgadmin/static/js/api_instance.js
echo
echo "== node_ajax.js relevant symbols/size =="
wc -l web/pgadmin/browser/static/js/node_ajax.js
sed -n '1,220p' web/pgadmin/browser/static/js/node_ajax.js
echo
echo "== all getApiInstance calls =="
rg -n -C2 'getApiInstance|_inflightGetRequests|apiInstance|axios\.get' web/pgadmin/static/js web/pgadmin -g '*.js'Repository: pgadmin-org/pgadmin4
Length of output: 50376
🌐 Web query:
Axios defaults request timeout default 0 infinite
💡 Result:
The default value for the Axios request timeout is 0 [1][2]. In Axios, a timeout value of 0 means there is no timeout, effectively causing the request to wait indefinitely [1][3][4]. It is highly recommended to set an explicit timeout value in production environments [1]. Without a defined timeout, a stalled or hanging request can remain open indefinitely, which may lead to resource exhaustion or unexpected application behavior [1][3]. When a request exceeds the specified timeout duration, Axios aborts the request and throws an error with the code 'ECONNABORTED' [1][4]. Note that the standard timeout property primarily governs response timeouts; it may not always account for connection-level issues, for which alternative approaches like AbortSignal are recommended [5].
Citations:
- 1: https://github.com/axios/axios
- 2: https://stackoverflow.com/questions/57995863/what-is-axios-default-timeout
- 3: Clarify in README that default timeout is 0 (no timeout) axios/axios#1750
- 4: https://apidog.com/blog/handle-axios-timeout/
- 5: https://stackoverflow.com/questions/68158083/how-to-handle-axios-timeout-with-hanging-api-server
Enforce a timeout before registering shared GET requests.
getInflight() stores the Axios promise before it settles, and the axios instance has no timeout/abort configuration, so a stalled request can retain the key forever and make later callers await the same pending request. Add a finite timeout (and/or cancellation handling) inside the request wrapper and clean the map on both abort and resolution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/pgadmin/static/js/api_instance.js` around lines 56 - 59, Update
getInflight() so each shared Axios GET request uses a finite timeout or
cancellation mechanism before being stored in _inflightGetRequests. Ensure the
map entry is removed when the request aborts, rejects, or resolves, preventing
stalled requests from retaining the key indefinitely.
…sions Address CodeRabbit review on pgadmin-org#10159. getInflight is exported, so key building should be safe regardless of caller, even though the sole caller today (getNodeAjaxOptions) can't trigger these cases: - Include config.headers in the key so callers differing only in headers (e.g. a different Authorization) never share an in-flight response. - Type-tag primitives in stableStringify so values with the same JSON form but different types no longer collide (number 1 vs string "1"), and handle undefined so [undefined] no longer collapses to []. Insertion-order independence and the undefined-params default are unchanged.
3 participants
Summary
Fixes #10155.
getNodeAjaxOptions()inweb/pgadmin/browser/static/js/node_ajax.js— the shared helper behind every AJAX-backed dropdown — cached responses only after they resolved. So when many components requested the same URL before the first response landed (e.g. every column row's Data Type dropdown mounting at once on a wide table's Columns tab), each one saw an empty cache and fired its own identicalGET. A 150-column table produced ~150 concurrent duplicateget_typesrequests.Fix
Added in-flight request sharing:
Maptracks requests currently in progress, keyed by the resolved full URL + query params..finally()), success or failure, so it doesn't leak.Result: one shared
get_typesrequest for all concurrent callers instead of one per row. Cached behavior and response-shape handling are unchanged.Notes
Could not run the JS linter locally — the repo's
.eslintrc.js(legacy format) is incompatible with the installed ESLint v9 (flat-config only), a pre-existing toolchain issue unrelated to this change. The file was verified to parse as valid ES module syntax.🤖 Generated with Claude Code
Summary by CodeRabbit