Skip to content

fix: dedupe concurrent getNodeAjaxOptions requests (#10155) #10159

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

Open
hiteshjambhale wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −9
Open

fix: dedupe concurrent getNodeAjaxOptions requests (#10155) #10159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 29 additions & 9 deletions web/pgadmin/browser/static/js/node_ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ export function generateNodeUrl(treeNodeInfo, actionType, itemNodeData, withId,
}


/* Tracks AJAX requests that are currently in flight, keyed by the resolved
* URL + query params. This lets concurrent callers asking for the same options
* (e.g. every row's Data Type dropdown mounting at once) share a single HTTP
* request instead of each firing their own before the cache is populated.
*/
const _inflightAjaxRequests = new Map();

/* Get the nodes list as options required by select controls
* The options are cached for performance reasons.
*/
Expand Down Expand Up @@ -114,15 +121,28 @@ export function getNodeAjaxOptions(url, nodeObj, treeNodeInfo, itemNodeData, par
let data = cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel);

if (_.isUndefined(data) || _.isNull(data)) {
api.get(fullUrl, {
params: otherParams.urlParams,
}).then((res)=>{
data = res.data;
if(res.data.data) {
data = res.data.data;
}
otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, data);
resolve(transform(data));
// 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 || {});

@mzabuawala mzabuawala Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some observations -

  1. 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.

  1. A better place to put such logic is inside the Axios wrapper, so that it will be used by the entire app

let request = _inflightAjaxRequests.get(inflightKey);
if (!request) {
request = api.get(fullUrl, {
params: otherParams.urlParams,
}).then((res)=>{
let resData = res.data;
if(res.data.data) {
resData = res.data.data;
}
otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
return resData;
}).finally(()=>{
_inflightAjaxRequests.delete(inflightKey);
});
_inflightAjaxRequests.set(inflightKey, request);
}
request.then((resData)=>{
resolve(transform(resData));
Comment on lines +137 to +145

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Move cache population outside the shared promise to avoid closure capture of the first caller's arguments.

The shared api.get(...).then(...) block captures the closure variables (otherParams, cacheNode, nodeObj, url, treeNodeInfo, cacheLevel) of the first caller. If concurrent callers request the same URL but differ in parameters (e.g., different nodeObj.type or useCache settings), the cache will only be populated for the first caller's key.

Moving the cacheNode.cache call into the individual request.then block ensures that every concurrent caller correctly executes its own caching logic and populates its respective cache entry before applying its transformation.

🐛 Proposed fix 
-            otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
             return resData;
           }).finally(()=>{
             _inflightAjaxRequests.delete(inflightKey);
           });
           _inflightAjaxRequests.set(inflightKey, request);
         }
         request.then((resData)=>{
+          if (otherParams.useCache) {
+            cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
+          }
           resolve(transform(resData));
📝 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.

Suggested change
otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
return resData;
}).finally(()=>{
_inflightAjaxRequests.delete(inflightKey);
});
_inflightAjaxRequests.set(inflightKey, request);
}
request.then((resData)=>{
resolve(transform(resData));
return resData;
}).finally(()=>{
_inflightAjaxRequests.delete(inflightKey);
});
_inflightAjaxRequests.set(inflightKey, request);
}
request.then((resData)=>{
if (otherParams.useCache) {
cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
}
resolve(transform(resData));
🤖 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/browser/static/js/node_ajax.js` around lines 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.

}).catch((err)=>{
reject(err instanceof Error ? err : Error('Something went wrong'));
});
Expand Down
Loading