Copilot commented on code in PR #18050:
URL:
https://github.com/apache/dolphinscheduler/pull/18050#discussion_r2917289741
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java:
##########
@@ -46,7 +48,16 @@ static Integer executeScript(String... cmd) {
inputStreamGobbler.start();
errorStreamGobbler.start();
- return process.waitFor();
+
+ boolean finished = process.waitFor(timeoutSeconds,
TimeUnit.SECONDS);
+ if (!finished) {
+ log.error("script execution timed out after {} seconds,
destroying process", timeoutSeconds);
+ process.destroyForcibly();
+ inputStreamGobbler.interrupt();
+ errorStreamGobbler.interrupt();
+ return -2;
Review Comment:
`timeoutSeconds` is documented as "if <= 0 waits indefinitely", but the
implementation always calls `process.waitFor(timeoutSeconds,
TimeUnit.SECONDS)`. For `timeoutSeconds <= 0` this will not wait indefinitely
and can immediately return `-2`. Handle `timeoutSeconds <= 0` by calling
`process.waitFor()` (no timeout) and only using timed wait when `timeoutSeconds
> 0`.
```suggestion
if (timeoutSeconds > 0) {
boolean finished = process.waitFor(timeoutSeconds,
TimeUnit.SECONDS);
if (!finished) {
log.error("script execution timed out after {} seconds,
destroying process", timeoutSeconds);
process.destroyForcibly();
inputStreamGobbler.interrupt();
errorStreamGobbler.interrupt();
return -2;
}
} else {
// Wait indefinitely when timeoutSeconds <= 0, as documented
process.waitFor();
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java:
##########
@@ -46,7 +48,16 @@ static Integer executeScript(String... cmd) {
inputStreamGobbler.start();
errorStreamGobbler.start();
- return process.waitFor();
+
+ boolean finished = process.waitFor(timeoutSeconds,
TimeUnit.SECONDS);
+ if (!finished) {
+ log.error("script execution timed out after {} seconds,
destroying process", timeoutSeconds);
+ process.destroyForcibly();
+ inputStreamGobbler.interrupt();
+ errorStreamGobbler.interrupt();
+ return -2;
+ }
+ return process.exitValue();
Review Comment:
On timeout the code interrupts the `StreamGobbler` threads, but
`StreamGobbler` blocks on `BufferedReader.readLine()` and doesn’t check the
interrupt flag, so `interrupt()` alone may not stop these threads promptly.
Consider closing the process streams and/or updating the gobblers to exit on
interrupt, and then `join()` them (or mark them as daemon) to avoid leaking
threads after a timeout.
```suggestion
try {
process.getInputStream().close();
} catch (IOException ioe) {
log.warn("Failed to close process input stream after
timeout", ioe);
}
try {
process.getErrorStream().close();
} catch (IOException ioe) {
log.warn("Failed to close process error stream after
timeout", ioe);
}
inputStreamGobbler.interrupt();
errorStreamGobbler.interrupt();
try {
inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
try {
errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
return -2;
}
int processExitCode = process.exitValue();
try {
inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
try {
errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
return processExitCode;
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java:
##########
@@ -100,4 +100,19 @@ public void testTypeIsError() {
assertFalse(alertResult.isSuccess());
}
+ @Test
+ public void testDefaultTimeout() {
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
+
+ @Test
+ public void testCustomTimeout() {
+ scriptConfig.put(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT, "30");
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
Review Comment:
`scriptConfig` is a static mutable map and `@BeforeEach` does not clear it.
After `testCustomTimeout` sets `NAME_SCRIPT_TIMEOUT`, later tests may
unintentionally reuse that value (test order is not guaranteed), making tests
flaky. Clear/recreate the map in `@BeforeEach` or remove the timeout key at the
end of this test.
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java:
##########
@@ -47,6 +48,9 @@ public final class ScriptSender {
userParams =
StringUtils.isNotBlank(config.get(ScriptParamsConstants.NAME_SCRIPT_USER_PARAMS))
? config.get(ScriptParamsConstants.NAME_SCRIPT_USER_PARAMS)
: "";
+ timeout =
StringUtils.isNotBlank(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))
+ ?
Long.parseLong(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))
+ : ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;
Review Comment:
`Long.parseLong(config.get(NAME_SCRIPT_TIMEOUT))` can throw
`NumberFormatException` if the stored plugin config contains a non-numeric
value. To prevent alert sending from failing with an uncaught exception, parse
this defensively (e.g., try/catch with fallback to `DEFAULT_SCRIPT_TIMEOUT`,
optionally logging a warning).
```suggestion
String timeoutConfig =
config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT);
if (StringUtils.isNotBlank(timeoutConfig)) {
long parsedTimeout =
ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;
try {
parsedTimeout = Long.parseLong(timeoutConfig);
} catch (NumberFormatException ex) {
log.warn("Invalid script timeout config value: '{}', using
default: {}",
timeoutConfig,
ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT, ex);
}
timeout = parsedTimeout;
} else {
timeout = ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;
}
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java:
##########
@@ -100,4 +100,19 @@ public void testTypeIsError() {
assertFalse(alertResult.isSuccess());
}
+ @Test
+ public void testDefaultTimeout() {
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
+
+ @Test
+ public void testCustomTimeout() {
+ scriptConfig.put(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT, "30");
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
Review Comment:
`testDefaultTimeout`/`testCustomTimeout` currently only assert that sending
succeeds, but they don’t validate that the configured timeout value is actually
parsed/applied. To meaningfully test the new behavior, consider using a
script/command that sleeps longer than the configured timeout and assert a
timeout failure, or mock/spying `ProcessUtils` to assert it’s called with the
expected timeout.
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java:
##########
@@ -33,7 +36,15 @@ public class ProcessUtilsTest {
@Test
public void testExecuteScript() {
- int code = ProcessUtils.executeScript(cmd);
+ int code = ProcessUtils.executeScript(60, cmd);
assert code != -1;
Review Comment:
This test uses the Java `assert` keyword, which is disabled by default in
Maven/JUnit runs unless `-ea` is set, so the assertion may never execute. Use
JUnit assertions (e.g., `Assertions.assertNotEquals(-1, code)` or a more
specific expected exit code) so the test reliably validates behavior.
```suggestion
Assertions.assertNotEquals(-1, code);
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactory.java:
##########
@@ -66,7 +68,17 @@ public List<PluginParams> params() {
.addValidate(Validate.newBuilder().setRequired(true).build())
.build();
- return Arrays.asList(scriptUserParam, scriptPathParam,
scriptTypeParams);
+ InputNumberParam scriptTimeoutParam =
+
InputNumberParam.newBuilder(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT,
+ ScriptParamsConstants.SCRIPT_TIMEOUT)
+ .setValue(ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT)
+ .addValidate(Validate.newBuilder()
+ .setType(DataType.NUMBER.getDataType())
+ .setRequired(false)
Review Comment:
The new timeout parameter allows any number, including negative values,
which can lead to unexpected behavior in `ProcessUtils` (and may cause
immediate timeout). Add a `min` validation (e.g., `setMin(0D)` or `setMin(1D)`
depending on whether 0 should disable timeouts) to constrain input in the
UI/config.
```suggestion
.setRequired(false)
.setMin(0D)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
