-
Notifications
You must be signed in to change notification settings - Fork 876
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
eae3f97
10512a4
73bfc89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,61 @@ export default function getApiInstance(headers={}) { | |
| }); | ||
| } | ||
|
|
||
| /* Tracks GET requests that are currently in flight, keyed by the resolved URL + | ||
| * params. This lets concurrent callers asking for the exact same resource | ||
| * (e.g. every column row's Data Type dropdown mounting at once on a wide table) | ||
| * share a single HTTP request instead of each firing their own identical GET. | ||
| */ | ||
| const _inflightGetRequests = new Map(); | ||
|
|
||
| /* Builds a key that is independent of object key insertion order, so that | ||
| * {a:1, b:2} and {b:2, a:1} (and nested variants) resolve to the same key. | ||
| * Params are expected to be plain JSON-like query values (primitives, plain | ||
| * objects, arrays); exotic inputs like Date/Map/cyclic objects are not valid | ||
| * GET query params and are out of scope here. | ||
| * Primitives are type-tagged so that values that share a JSON form but differ | ||
| * in type never collide, e.g. the number 1 vs the string "1", or an array hole | ||
| * ([undefined]) vs the empty array ([]). */ | ||
| function stableStringify(value) { | ||
| if (value === undefined) { | ||
| return 'undefined'; | ||
| } | ||
| if (value === null || typeof value !== 'object') { | ||
| return typeof value + ':' + JSON.stringify(value); | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return '[' + value.map(stableStringify).join(',') + ']'; | ||
| } | ||
| return '{' + Object.keys(value).sort().map( | ||
| (key) => JSON.stringify(key) + ':' + stableStringify(value[key]) | ||
| ).join(',') + '}'; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /* Like api.get(url, config), but shares a single in-flight request among all | ||
| * concurrent callers requesting the same url + params. The shared entry is | ||
| * removed once the request settles (success or failure), so it never leaks and | ||
| * later calls fetch fresh data. Each caller attaches its own then/catch, so | ||
| * response handling (transform, caching, etc.) stays per-caller. */ | ||
| export function getInflight(api, url, config={}) { | ||
| // Key on url + params + any per-request headers, so callers that differ only | ||
| // in headers (e.g. a different Authorization) never share a response. The | ||
| // instance-level headers set by getApiInstance() are the session-global CSRF | ||
| // token and Content-type, identical across concurrent callers, so they don't | ||
| // need to be in the key; anything caller-specific should be passed here via | ||
| // config.headers. | ||
| const key = url | ||
| + '#' + stableStringify(config.params ?? {}) | ||
| + '#' + stableStringify(config.headers ?? {}); | ||
| let request = _inflightGetRequests.get(key); | ||
| if (!request) { | ||
| request = api.get(url, config).finally(() => { | ||
| _inflightGetRequests.delete(key); | ||
| }); | ||
| _inflightGetRequests.set(key, request); | ||
|
Comment on lines
+73
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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:
💡 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:
Enforce a timeout before registering shared GET requests.
🤖 Prompt for AI Agents |
||
| } | ||
| return request; | ||
| } | ||
|
|
||
| export function parseApiError(error, withData=false) { | ||
| if (error.response) { | ||
| // The request was made and the server responded with a status code | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.