-
-
Notifications
You must be signed in to change notification settings - Fork 12
Add timeout #147
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
Add timeout #147
Changes from all commits
c13754d
456a2f1
2040f45
115c25e
ad39b01
3b79cf1
9d1d97e
ea5297e
0b98883
24fc64e
962b23e
61cc107
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -69,12 +69,14 @@ class JavaCaller { | |||||
| * Runs java command of a JavaCaller instance | ||||||
| * @param {string[]} [userArguments] - Java command line arguments | ||||||
| * @param {object} [runOptions] - Run options | ||||||
| * @param {boolean} [runOptions.detached = false] - If set to true, node will node wait for the java command to be completed. In that case, childJavaProcess property will be returned, but stdout and stderr may be empty | ||||||
| * @param {boolean} [runOptions.detached = false] - If set to true, node will not wait for the java command to be completed. In that case, childJavaProcess property will be returned, but stdout and stderr may be empty | ||||||
| * @param {string} [runOptions.stdoutEncoding = 'utf8'] - Adds control on spawn process stdout | ||||||
| * @param {number} [runOptions.waitForErrorMs = 500] - If detached is true, number of milliseconds to wait to detect an error before exiting JavaCaller run | ||||||
| * @param {string} [runOptions.cwd = .] - You can override cwd of spawn called by JavaCaller runner | ||||||
| * @param {string} [runOptions.javaArgs = []] - You can override cwd of spawn called by JavaCaller runner | ||||||
| * @param {string} [runOptions.windowsVerbatimArguments = true] - No quoting or escaping of arguments is done on Windows. Ignored on Unix. This is set to true automatically when shell is specified and is CMD. | ||||||
| * @param {number} [runOptions.timeout] - In milliseconds the maximum amount of time the process is allowed to run | ||||||
| * @param {NodeJS.Signals | number} [runOptions.killSignal = "SIGTERM"] - The signal value to be used when the spawned process will be killed by timeout or abort signal. | ||||||
| * @return {Promise<{status:number, stdout:string, stderr:string, childJavaProcess:ChildProcess}>} - Command result (status, stdout, stderr, childJavaProcess) | ||||||
| */ | ||||||
| async run(userArguments, runOptions = {}) { | ||||||
|
|
@@ -84,6 +86,7 @@ class JavaCaller { | |||||
| runOptions.stdoutEncoding = typeof runOptions.stdoutEncoding === "undefined" ? "utf8" : runOptions.stdoutEncoding; | ||||||
| runOptions.windowsVerbatimArguments = typeof runOptions.windowsVerbatimArguments === "undefined" ? true : runOptions.windowsVerbatimArguments; | ||||||
| runOptions.windowless = typeof runOptions.windowless === "undefined" ? false : os.platform() !== "win32" ? false : runOptions.windowless; | ||||||
| runOptions.killSignal = typeof runOptions.killSignal === "undefined" ? "SIGTERM" : runOptions.killSignal; | ||||||
| this.commandJavaArgs = (runOptions.javaArgs || []).concat(this.additionalJavaArgs); | ||||||
|
|
||||||
| let javaExe = runOptions.windowless ? this.javaExecutableWindowless : this.javaExecutable; | ||||||
|
|
@@ -97,10 +100,13 @@ class JavaCaller { | |||||
|
|
||||||
| const javaExeToUse = this.javaExecutableFromNodeJavaCaller ?? javaExe; | ||||||
| const classPathStr = this.buildClasspathStr(); | ||||||
| const javaArgs = this.buildArguments(classPathStr, (userArguments || []).concat(this.commandJavaArgs)); | ||||||
| const javaArgs = this.buildArguments(classPathStr, (userArguments || []).concat(this.commandJavaArgs), runOptions.windowsVerbatimArguments); | ||||||
| let stdout = ""; | ||||||
| let stderr = ""; | ||||||
| let child; | ||||||
| let timeoutId; | ||||||
| let killedByTimeout = false; | ||||||
|
|
||||||
| const prom = new Promise((resolve) => { | ||||||
| // Spawn java command line | ||||||
| debug(`Java command: ${javaExeToUse} ${javaArgs.join(" ")}`); | ||||||
|
|
@@ -117,6 +123,19 @@ class JavaCaller { | |||||
| } | ||||||
| child = spawn(javaExeToUse, javaArgs, spawnOptions); | ||||||
|
|
||||||
| if (runOptions.timeout) { | ||||||
| timeoutId = setTimeout(() => { | ||||||
| if (!child.killed) { | ||||||
| try { | ||||||
| child.kill(runOptions.killSignal); | ||||||
| killedByTimeout = true; | ||||||
| } catch (err) { | ||||||
| stderr += `Failed to kill process after ${runOptions.timeout}ms: ${err.message}`; | ||||||
| } | ||||||
| } | ||||||
| }, runOptions.timeout); | ||||||
| } | ||||||
|
|
||||||
| // Gather stdout and stderr if they must be returned | ||||||
| if (spawnOptions.stdio === "pipe") { | ||||||
| child.stdout.setEncoding(`${runOptions.stdoutEncoding}`); | ||||||
|
|
@@ -132,12 +151,28 @@ class JavaCaller { | |||||
| child.on("error", (data) => { | ||||||
| this.status = 666; | ||||||
| stderr += "Java spawn error: " + data; | ||||||
|
|
||||||
| if (timeoutId) { | ||||||
| clearTimeout(timeoutId); | ||||||
| } | ||||||
|
|
||||||
| resolve(); | ||||||
| }); | ||||||
|
|
||||||
| // Catch status code | ||||||
| child.on("close", (code) => { | ||||||
| this.status = code; | ||||||
| if (timeoutId) { | ||||||
| clearTimeout(timeoutId); | ||||||
| } | ||||||
|
|
||||||
| if (killedByTimeout) { | ||||||
| // Process was terminated because of the timeout | ||||||
| this.status = 666; | ||||||
| stderr += `Process timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`; | ||||||
|
||||||
| stderr += `Process timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`; | |
| stderr += `\nProcess timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`; |
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 implementation creates both a fallback setTimeout timer and passes timeout to the spawn options, which means Node.js's built-in timeout mechanism will also be active. This could result in redundant timeout handling. The built-in spawn timeout should be sufficient in most cases. Consider removing the custom setTimeout implementation (lines 147-158) since spawn already handles timeout internally, or document why both mechanisms are necessary.