Add timeout by BrendanC23 · Pull Request #147 · nvuillam/node-java-caller

BrendanC23 · 2026-02-02T03:40:19Z

Fixes #144

Proposed Changes

Add timeout and killSignal options

This commit adds new runOptions.timeout and runOptions.killSignal options that allow a process to be killed after a specified number of milliseconds have elapsed. See the Node ChildProcess docs.

This commit includes several additional changes that were necessary as part of the implementation of timeout:

Fix to JavaCallerTester.java that replaces an == string comparison with a args[0].equals("--sleep"). This fixes the sleep command line argument, which previously never triggered because the comparison was always false.
Recompile JavaCallerTester.class and JavaCallerTester.jar
Fixed should call JavaCallerTester.class detached test. This began failing after the call fix for --sleep. The test has been rewritten to demonstrate the expected behavior for detached. Now, the test will check that the Java process is initially still running and then check the return status code once it exits.
Update the NPM script java:compile to use --release 8 instead of -source 8 -target 1.8. This fixes an error that occurred when running the previous script:

warning: [options] bootstrap class path is not set in conjunction with -source 8
not setting the bootstrap class path may lead to class files that cannot run on JDK 8
--release 8 is recommended instead of -source 8 -target 1.8 because it sets the bootstrap class path automatically

See this StackOverflow post for more details.

Note that a warning is still generated:

warning: [options] target value 8 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.

Readiness Checklist

Author/Contributor

Add entry to the CHANGELOG.md listing the change and linking to the corresponding issue (if appropriate)
If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

Label as breaking if this is a large fundamental change
Label as either automation, bug, documentation, enhancement, infrastructure, or performance

This commit adds new `runOptions.timeout` and `runOptions.killSignal` options that allow a process to be killed after a specified number of milliseconds have elapsed. See the [Node ChildProcess docs](https://nodejs.org/api/child_process.html#child_processspawncommand-args-options). This commit includes several additional changes that were necessary as part of the implementation of `timeout`: * Fix to `JavaCallerTester.java` that replaces an `==` string comparison with a `args[0].equals("--sleep")`. This fixes the `sleep` command line argument, which previously never triggered because the comparison was always false. * Recompile `JavaCallerTester.class` and `JavaCallerTester.jar` * Fixed `should call JavaCallerTester.class detached` test. This began failing after the call fix for `--sleep`. The test has been rewritten to demonstrate the expected behavior for `detached`. Now, the test will check that the Java process is initially still running and then check the return status code once it exits. * Update the NPM script `java:compile` to use `--release 8` instead of `-source 8 -target 1.8`. This fixes an error that occurred when running the previous script: >warning: [options] bootstrap class path is not set in conjunction with -source 8 not setting the bootstrap class path may lead to class files that cannot run on JDK 8 --release 8 is recommended instead of -source 8 -target 1.8 because it sets the bootstrap class path automatically See [this StackOverflow post](https://stackoverflow.com/a/61715683) for more details. Note that a warning is still generated: >warning: [options] target value 8 is obsolete and will be removed in a future release warning: [options] To suppress warnings about obsolete options, use -Xlint:-options. Fixes nvuillam#144

nvuillam · 2026-02-03T10:20:18Z

@BrendanC23 looks great :)
Please can you resolve conflicts ?

This commit removes the check for `exitCode === null`, which will only be the case on Windows. On Linux, the `exitCode` will be populated. The [Node docs](https://nodejs.org/api/child_process.html#event-exit), say: >The exit code if the child process exited on its own, or null if the child process terminated due to a signal. >The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null.

nvuillam · 2026-02-04T04:11:28Z

@BrendanC23 it seems to work great on winfows, but not on mac / ubuntu, please can you check CI job logs ?

BrendanC23 · 2026-02-04T04:12:55Z

It looks like on Linux, code won't be null if the process is killed by a signal. That doesn't seem to match the Node docs, which say:

The exit code if the child process exited on its own, or null if the child process terminated due to a signal.
The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null.

Instead, the code is 128 + SIGNAL. Oddly, this behavior didn't show up locally on WSL. I'm working on a fix.

As recommended in the [TypeScript ESLint docs](https://typescript-eslint.io/troubleshooting/faqs/eslint#i-get-errors-from-the-no-undef-rule-about-global-variables-not-being-defined-even-though-there-are-no-typescript-errors), the `no-undef` rule should not be used for `*.ts` files because it gives false positives. This causes issues with using `NodeJS.Signals` in `index.d.ts`.

BrendanC23 · 2026-02-04T04:47:35Z

I'm still working on a full fix for the failing tests. Apparently this line still isn't executing.

I turned off the no-undef rule for *.ts files because it was giving false positives. See this suggestion in the docs

I noticed that ESLint isn't running locally. A previous PR updated it to version 9, which requires a flat config file. I was also getting an error that @typescript-eslint/parser wasn't found, because typescript-eslint isn't included in the package.json.

BrendanC23 · 2026-02-04T05:07:28Z

I added some logging. On Linux, for the failing tests, { code: 130, signal: null } instead of { code: null, signal: "SIGINT" }.

nvuillam · 2026-02-04T06:47:52Z

@copilot can you help make the timeout feature work on mac and linux ?

nvuillam · 2026-02-04T11:05:39Z

@BrendanC23 Copilot can not act on forked repos pull requests, do you have it ? (it should work even with the free tier)

codecov-commenter · 2026-02-04T21:02:17Z

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.38%. Comparing base (639f49b) to head (61cc107).

Files with missing lines	Patch %	Lines
lib/java-caller.js	83.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
- Coverage   89.95%   89.38%   -0.57%     
==========================================
  Files           3        3              
  Lines         229      245      +16     
==========================================
+ Hits          206      219      +13     
- Misses         23       26       +3

Flag	Coverage Δ
unittests	`89.38% <83.33%> (-0.57%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR adds support for Node.js spawn's timeout and killSignal options to the JavaCaller library, addressing issue #144. The implementation allows users to specify a maximum execution time for Java processes and control which signal is used to terminate them when the timeout is reached.

Changes:

Added timeout and killSignal options to JavaCallerRunOptions
Fixed a bug in JavaCallerTester.java where string comparison used == instead of .equals()
Updated Java compilation command to use --release 8 instead of deprecated -source 8 -target 1.8 flags
Modified detached mode test to properly demonstrate and verify detached process behavior

Reviewed changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
lib/java-caller.js	Implements timeout functionality with dual mechanism (built-in spawn timeout and custom setTimeout), adds signal detection logic, and handles timeout status codes
lib/index.d.ts	Adds TypeScript type definitions for new timeout and killSignal options
test/java-caller.test.js	Updates detached test to verify proper behavior and adds three new tests for timeout functionality
test/java/src/com/nvuillam/javacaller/JavaCallerTester.java	Fixes string comparison bug and changes sleep duration from minutes to milliseconds
test/java/jar/*.jar	Recompiled JAR files with bug fixes
test/java/dist/*.class	Recompiled class files with bug fixes
package.json	Updates Java compilation command to use modern `--release 8` flag
.eslintrc.js	Disables `no-undef` rule for TypeScript definition files
README.md	Adds documentation for new timeout and killSignal options
CHANGELOG.md	Documents the new feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T21:16:23Z

test/java-caller.test.js

        checkStdOutIncludes(`JavaCallerTester is called !`, stdout, stderr);
    });
+
+        it("should terminate once timeout is reached", async () => {


Inconsistent indentation: this test block has incorrect indentation (extra leading spaces) compared to other test blocks in the file. It should start at the same indentation level as the other it blocks.

Suggested change

it("should terminate once timeout is reached", async () => {

it("should terminate once timeout is reached", async () => {

Copilot · 2026-02-04T21:16:24Z

lib/java-caller.js

+            if (killedByTimeout) {
+                return true;
+            }
+            if (signal && signal === runOptions.killSignal) {
+                return true;
+            }
+            const signals = os.constants && os.constants.signals ? os.constants.signals : {};
+            if (typeof runOptions.killSignal === "string" && signals[runOptions.killSignal] && code === 128 + signals[runOptions.killSignal]) {
+                return true;
+            }
+            if (typeof runOptions.killSignal === "number" && code === 128 + runOptions.killSignal) {
+                return true;
+            }
+            return false;


The wasKilledByTimeout function may incorrectly identify a process as timed out if it naturally exits with a signal that matches killSignal without actually being killed by the timeout. For example, if a Java process catches and handles SIGTERM gracefully and exits with that signal code, it would be incorrectly identified as timed out. Consider only relying on the killedByTimeout flag when it's set, and for other cases, only check the signal if you're certain the process was actually killed (e.g., by checking if the signal matches AND the timeout has actually elapsed).

Suggested change

if (killedByTimeout) {

return true;

}

if (signal && signal === runOptions.killSignal) {

return true;

}

const signals = os.constants && os.constants.signals ? os.constants.signals : {};

if (typeof runOptions.killSignal === "string" && signals[runOptions.killSignal] && code === 128 + signals[runOptions.killSignal]) {

return true;

}

if (typeof runOptions.killSignal === "number" && code === 128 + runOptions.killSignal) {

return true;

}

return false;

return killedByTimeout;

Copilot · 2026-02-04T21:16:24Z

lib/java-caller.js

            child.on("error", (data) => {
                this.status = 666;
                stderr += "Java spawn error: " + data;
                resolve();
            });


If a spawn error occurs (line 172), the timeout timer is not cleared before resolving. This means the timer will continue to run and attempt to kill the process even after the error handler has completed. The timeout should be cleared in the error handler as well to prevent this leak.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-04T21:16:25Z

lib/java-caller.js

+                if (wasKilledByTimeout(code, signal)) {
+                    // Process was terminated because of the timeout, either via our fallback timer or the built-in spawn timeout
+                    this.status = 666;
+                    stderr += `Process timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`;


The error message uses template literal syntax but doesn't include a newline or proper formatting. When concatenating to existing stderr content, this could result in the timeout message being appended directly to previous error output without separation. Consider adding a newline at the beginning of the message for better readability, similar to how other error messages are formatted in this codebase.

Suggested change

stderr += `Process timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`;

stderr += `\nProcess timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`;

Copilot · 2026-02-04T21:16:25Z

lib/java-caller.js

+            if (runOptions.timeout) {
+                timeoutId = setTimeout(() => {
+                    if (!child.killed) {
+                        killedByTimeout = true;
+                        try {
+                            child.kill(runOptions.killSignal);
+                        } catch (err) {
+                            stderr += `Failed to kill process after ${runOptions.timeout}ms: ${err.message}`;
+                        }
+                    }
+                }, runOptions.timeout);
+            }


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.

Copilot · 2026-02-04T21:16:25Z

lib/java-caller.js

+            const signals = os.constants && os.constants.signals ? os.constants.signals : {};
+            if (typeof runOptions.killSignal === "string" && signals[runOptions.killSignal] && code === 128 + signals[runOptions.killSignal]) {
+                return true;
+            }
+            if (typeof runOptions.killSignal === "number" && code === 128 + runOptions.killSignal) {
+                return true;
+            }


The logic for detecting timeout based on exit code (128 + signal number) is Unix-specific and may not work correctly on Windows. On Windows, processes terminated by signals typically exit with different status codes. Consider testing this behavior on Windows and handling platform-specific differences, or rely solely on the killedByTimeout flag and signal parameter for detection.

@copilot open a new pull request to apply changes based on this feedback

This commit simplifies the timeout logic by no longer passing `timeout` to `spawn`. Instead, the full timeout logic is handled by `run`, which sets a timeout and determines whether the process was killed. Now that there is no duplicate logic, `wasKilledByTimeout` can be removed in favor of checking `killedByTimeout` directly.

BrendanC23 · 2026-02-06T03:26:50Z

@nvuillam Can you review this? Your plan to manually handle the timeout instead of passing it to spawn works well. I don't think we need both, and removing the latter simplifies the logic to determine when a process was killed due to a timeout.

nvuillam

Looks fine to me :)
Thanks for the PR ,I'll merge and release later today or this week-end :)

BrendanC23 added 2 commits February 1, 2026 21:07

Add to changelog

456a2f1

BrendanC23 added 3 commits February 3, 2026 21:15

Merge remote-tracking branch 'upstream/main' into add-timeout

2040f45

Add timeout options to JavaCallerRunOptions type

115c25e

BrendanC23 added 2 commits February 3, 2026 22:25

Fix killSignal default in TypeScript docs

3b79cf1

Add temporary logging

ea5297e

nvuillam added 2 commits February 4, 2026 21:55

Handle cross-platform timeout

0b98883

unify timeout codes

24fc64e

nvuillam requested a review from Copilot February 4, 2026 21:12

Copilot started reviewing on behalf of nvuillam February 4, 2026 21:12 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

BrendanC23 added 2 commits February 5, 2026 20:15

Remove unused variable

61cc107

nvuillam approved these changes Feb 6, 2026

View reviewed changes

nvuillam merged commit bbe6af2 into nvuillam:main Feb 7, 2026
45 checks passed

BrendanC23 deleted the add-timeout branch February 7, 2026 15:53

	it("should terminate once timeout is reached", async () => {
	it("should terminate once timeout is reached", async () => {

	stderr += `Process timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`;
	stderr += `\nProcess timed out with ${runOptions.killSignal} after ${runOptions.timeout}ms.`;

Uh oh!

Conversation

BrendanC23 commented Feb 2, 2026

Proposed Changes

Readiness Checklist

Author/Contributor

Reviewing Maintainer

Uh oh!

nvuillam commented Feb 3, 2026

Uh oh!

nvuillam commented Feb 4, 2026

Uh oh!

BrendanC23 commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrendanC23 commented Feb 4, 2026

Uh oh!

BrendanC23 commented Feb 4, 2026

Uh oh!

nvuillam commented Feb 4, 2026

Uh oh!

nvuillam commented Feb 4, 2026

Uh oh!

codecov-commenter commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

nvuillam Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

nvuillam Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

BrendanC23 commented Feb 6, 2026

Uh oh!

nvuillam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BrendanC23 commented Feb 4, 2026 •

edited

Loading

codecov-commenter commented Feb 4, 2026 •

edited

Loading