-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix: Abort fetch requests on timeout in SimStudioClien #656
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: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@SyedaAnshrahGillani is attempting to deploy a commit to the Sim Studio Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Enhanced request timeout handling in SimStudioClient by implementing AbortController for fetch requests, ensuring proper cleanup of network resources and preventing memory leaks.
- Replaced Promise.race implementation with AbortController in
packages/ts-sdk/src/index.ts
for better request cancellation - Added signal propagation to fetch options for proper request termination
- Implemented clearTimeout cleanup to prevent stale timeout handlers
- Tests confirm proper timeout handling with 408 status codes and error messages
1 file reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
try { | ||
const fetchPromise = fetch(url, { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
'X-API-Key': this.apiKey, | ||
}, | ||
body: JSON.stringify(input || {}), | ||
signal: abortController.signal, // Attach the abort signal | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Remove nested try block - already inside a try block. Move catch handler up to avoid nested error handling.
try { | |
const fetchPromise = fetch(url, { | |
method: 'POST', | |
headers: { | |
'Content-Type': 'application/json', | |
'X-API-Key': this.apiKey, | |
}, | |
body: JSON.stringify(input || {}), | |
signal: abortController.signal, // Attach the abort signal | |
}) | |
const fetchPromise = fetch(url, { | |
method: 'POST', | |
headers: { | |
'Content-Type': 'application/json', | |
'X-API-Key': this.apiKey, | |
}, | |
body: JSON.stringify(input || {}), | |
signal: abortController.signal, // Attach the abort signal | |
}) |
} catch (error: any) { | ||
if (error instanceof SimStudioError) { | ||
throw error | ||
} | ||
clearTimeout(timeoutId) // Ensure timeout is cleared on error too | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: clearTimeout(timeoutId) is indented incorrectly, making it unreachable for some error paths. Move to same level as catch block.
} catch (error: any) { | |
if (error instanceof SimStudioError) { | |
throw error | |
} | |
clearTimeout(timeoutId) // Ensure timeout is cleared on error too | |
} catch (error: any) { | |
clearTimeout(timeoutId) // Ensure timeout is cleared on error too |
Summary
This PR addresses a critical resource management and performance issue in the
SimStudioClient
by implementing proper request abortion for timed-outfetch
calls.Problem
Previously, the
executeWorkflow
method usedPromise.race
to handle timeouts. While this approach rejected the timeout promise, the underlyingfetch
request continued running in the background, leading to:Solution
The method is refactored to use the
AbortController
API for more robust timeout handling:AbortController
is created for eachfetch
request.setTimeout
is used to triggerabortController.abort()
after the configured timeout period.signal
is passed to thefetch
options to enable cancellation.clearTimeout
is called once the request completes, preventing stale timeouts.AbortError
and provide clear messaging for timeout events.Benefits