-
Notifications
You must be signed in to change notification settings - Fork 2
JSON response error handling + abort timeout handling #145
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
Conversation
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.
Pull Request Overview
This PR improves error handling and response management for JSON API responses by switching from streamed to buffered JSON responses and enhancing timeout/abort scenarios. The changes focus on making the system more resilient to network issues and providing better error handling during code execution polling.
- Refactors JSON response handling to use buffered responses instead of streamed ones
- Adds error handling with try-catch blocks for execution polling operations
- Enhances logging by including execution context in error messages
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ui.frontend/src/hooks/execution.ts | Adds error handling for polling operations and enables quiet mode for API requests |
ui.frontend/src/components/ExecutionAbortButton.tsx | Enhances error logging by including execution context |
core/src/main/java/dev/vml/es/acm/core/util/ServletUtils.java | Refactors JSON response methods to use buffered responses by default |
core/src/main/java/dev/vml/es/acm/core/servlet/ExecuteCodeServlet.java | Updates to use new buffered JSON response method |
core/src/main/java/dev/vml/es/acm/core/servlet/DescribeCodeServlet.java | Updates to use new buffered JSON response method |
await new Promise((resolve) => setTimeout(resolve, pollInterval)); | ||
} | ||
|
||
return queuedExecution; | ||
return queuedExecution!; |
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.
The non-null assertion operator (!) is unsafe here. If an error occurs in the try-catch block and queuedExecution remains null, this will cause a runtime error. Consider returning null or throwing a specific error instead.
return queuedExecution!; | |
if (queuedExecution === null) { | |
throw new Error('Failed to retrieve a valid execution state after polling.'); | |
} | |
return queuedExecution; |
Copilot uses AI. Check for mistakes.
queuedExecution = response.data.data.executions[0]!; | ||
} catch (error) { | ||
console.warn('Code execution pending state unknown:', error); |
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.
The setTimeout is called regardless of whether the API request succeeded or failed. This means the function will continue polling indefinitely even after errors, potentially creating an infinite loop if the API consistently fails.
queuedExecution = response.data.data.executions[0]!; | |
} catch (error) { | |
console.warn('Code execution pending state unknown:', error); | |
queuedExecution = response.data.data.executions[0]!; | |
retryCount = 0; // Reset retry counter on success | |
} catch (error) { | |
console.warn('Code execution pending state unknown:', error); | |
retryCount++; | |
if (retryCount >= maxRetries) { | |
throw new Error(`Failed to fetch execution state after ${maxRetries} retries.`); | |
} |
Copilot uses AI. Check for mistakes.
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.
that's expected; temporary fails are possible
public static void respondJson(SlingHttpServletResponse response, ServletResult<?> result) throws IOException { | ||
respondJsonBuffered(response, result); |
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.
This change from streaming to buffered JSON responses is a breaking change in behavior. The old streaming method consumed less memory, and switching all responses to buffered mode could impact performance for large responses without providing a migration path.
public static void respondJson(SlingHttpServletResponse response, ServletResult<?> result) throws IOException { | |
respondJsonBuffered(response, result); | |
public static void respondJson(SlingHttpServletResponse response, ServletResult<?> result, boolean useBuffered) throws IOException { | |
if (useBuffered) { | |
respondJsonBuffered(response, result); | |
} else { | |
respondJsonStreamed(response, result); | |
} |
Copilot uses AI. Check for mistakes.
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.
but improves error handling; now when something breaks the malformed JSON is output
No description provided.