fix: correct PID check, preserve stack traces, and add missing return after queue overflow reject#110
Conversation
… after queue overflow reject Signed-off-by: Swastik Prakash <swastikprakashofficial@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens up error handling and edge cases in the JavaScript evaluation pipeline, improving debuggability and correctness when handling worker processes and queue limits.
Changes:
- Added
stacksupport toEvalResponseand included stack traces in some rejection paths. - Ensured
evalChildProcessstops after rejecting due tomaxQueueDepthoverflow. - Corrected the child-process PID check so PID
0isn’t treated as missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (err: any) { | ||
| console.log(err); | ||
| reject({ |
| if(this.queue.length >= this.options.maxQueueDepth) { | ||
| reject({ maxQueueDepthExceeded: true, elapsed: 0 }); | ||
| return; | ||
| } |
| const workerPath = this.getWorkerPath(); | ||
| // console.debug(`Worker path: ${workerPath}`); | ||
| const worker = child_process.fork(workerPath, { timeout: options.timeout, env: {} }); | ||
| if (!worker.pid) { | ||
| if (worker.pid === undefined || worker.pid === null) { |
Sana9058
left a comment
Sana9058
left a comment
There was a problem hiding this comment.
Hi! I reviewed this PR, and the changes for PID validation, stack trace handling, and stopping execution after queue overflow look correct and align with the expected behavior.
One small observation: in a few places, errors are logged using console.log(err). It might be better to use console.error(err) for clearer error reporting, as mentioned in earlier comments.
Overall, the implementation looks good to me. Thanks for working on this!
| @@ -137,15 +138,17 @@ export class JavaScriptEvaluator { | |||
| .catch(err => { | |||
| console.log(err); | |||
There was a problem hiding this comment.
One small observation:
In this catch block, console.log(err) is being used.
Since errors were switched to console.error in other places,
it might be better to use console.error(err) here as well for consistency.
Let me know what you think.
Fix some edge cases & error handling
Fixed PID check so 0 isn’t treated as missing
Switched to console.error and added stack traces for better debugging
Stopped execution after queue overflow reject (was still pushing to queue)