Copilot commented on code in PR #18050:
URL:
https://github.com/apache/dolphinscheduler/pull/18050#discussion_r3013125822
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java:
##########
@@ -46,12 +51,53 @@ static Integer executeScript(String... cmd) {
inputStreamGobbler.start();
errorStreamGobbler.start();
- return process.waitFor();
- } catch (IOException | InterruptedException e) {
- log.error("execute alert script error {}", e.getMessage());
+
+ 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();
+ closeProcessStreams(process);
+ joinGobbler(inputStreamGobbler);
+ joinGobbler(errorStreamGobbler);
+ return EXECUTE_TIMEOUT_EXIT_CODE;
+ }
+ } else {
+ process.waitFor();
+ }
+ int processExitCode = process.exitValue();
+ joinGobbler(inputStreamGobbler);
+ joinGobbler(errorStreamGobbler);
+ return processExitCode;
+ } catch (InterruptedException e) {
+ log.error("execute alert script interrupted {}", e.getMessage());
Thread.currentThread().interrupt();
+ } catch (IOException e) {
+ log.error("execute alert script error {}", e.getMessage());
}
return exitCode;
}
+
+ private static void closeProcessStreams(Process process) {
+ try {
+ process.getInputStream().close();
+ } catch (IOException e) {
+ log.warn("Failed to close process input stream after timeout", e);
+ }
+ try {
+ process.getErrorStream().close();
+ } catch (IOException e) {
+ log.warn("Failed to close process error stream after timeout", e);
+ }
+ }
+
+ private static void joinGobbler(StreamGobbler gobbler) {
+ try {
+ gobbler.interrupt();
+ gobbler.join(TimeUnit.SECONDS.toMillis(1));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
Review Comment:
`joinGobbler()` interrupts the `StreamGobbler` thread and only waits 1
second. However `StreamGobbler.run()` blocks on `BufferedReader.readLine()` and
does not check interruption, so the interrupt is ineffective and this method
can return while gobbler threads are still running. That can lead to thread
accumulation under heavy alert load (or very large outputs). Consider making
`StreamGobbler` interruption-aware (break loop on interrupt / handle
IOException from closed stream), setting gobbler threads as daemon, and/or
joining without a fixed 1s timeout after the process has exited and streams are
closed.
```suggestion
gobbler.join();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
log.warn("Interrupted while waiting for stream gobbler to
finish", e);
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactory.java:
##########
@@ -66,7 +68,18 @@ 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)
+ .setMin(0D)
Review Comment:
`executeScript()` treats `timeoutSeconds <= 0` as “wait indefinitely” (see
Javadoc), but the new UI param validation allows `0` (`setMin(0D)`), and
`ScriptSender.parseTimeout()` doesn’t reject `0`/negative values. This means a
misconfiguration can disable the timeout entirely and reintroduce the original
“hang forever” problem. If disabling timeouts isn’t an intended feature,
enforce `timeout >= 1` (UI + backend) or fall back to the default when `timeout
<= 0`; if it is intended, please document that `0` disables the timeout.
```suggestion
.setMin(1D)
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java:
##########
@@ -100,4 +114,46 @@ public void testTypeIsError() {
assertFalse(alertResult.isSuccess());
}
+ @Test
+ public void testDefaultTimeout() {
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ if (isWindows()) {
+ Assertions.assertFalse(alertResult.isSuccess());
+ Assertions.assertEquals("shell script not support windows os",
alertResult.getMessage());
+ } else {
+ 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");
+ if (isWindows()) {
+ Assertions.assertFalse(alertResult.isSuccess());
+ Assertions.assertEquals("shell script not support windows os",
alertResult.getMessage());
+ } else {
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
+ }
+
Review Comment:
`testDefaultTimeout()` and `testCustomTimeout()` are effectively duplicates
of the existing happy-path test and don’t assert that the configured timeout is
actually parsed and applied (e.g., by executing a script that sleeps longer
than the configured timeout and asserting a timeout result). As written, these
tests would still pass even if the timeout value were ignored.
```suggestion
```
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java:
##########
@@ -100,4 +114,46 @@ public void testTypeIsError() {
assertFalse(alertResult.isSuccess());
}
+ @Test
+ public void testDefaultTimeout() {
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ if (isWindows()) {
+ Assertions.assertFalse(alertResult.isSuccess());
+ Assertions.assertEquals("shell script not support windows os",
alertResult.getMessage());
+ } else {
+ 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");
+ if (isWindows()) {
+ Assertions.assertFalse(alertResult.isSuccess());
+ Assertions.assertEquals("shell script not support windows os",
alertResult.getMessage());
+ } else {
+ Assertions.assertTrue(alertResult.isSuccess());
+ }
+ }
+
+ @Test
+ public void testInvalidTimeoutFallsBackToDefault() {
+ scriptConfig.put(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT,
"notANumber");
+ ScriptSender scriptSender = new ScriptSender(scriptConfig);
+ AlertResult alertResult = scriptSender.sendScriptAlert("test title
Kris", "test content");
+ Assertions.assertFalse(alertResult.isSuccess());
+ if (isWindows()) {
+ Assertions.assertEquals("shell script not support windows os",
alertResult.getMessage());
+ } else {
+ Assertions.assertEquals("script timeout config is invalid, should
be a number", alertResult.getMessage());
+ }
+ }
Review Comment:
The test name `testInvalidTimeoutFallsBackToDefault` suggests that an
invalid timeout should fall back to the default, but the assertions expect the
send to fail with an “invalid config” message (on non-Windows). Either rename
this test to reflect the current behavior, or adjust the implementation to
actually fall back to the default timeout as the name implies.
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java:
##########
@@ -106,18 +108,42 @@ private AlertResult executeShellScript(String title,
String content) {
return alertResult;
}
+ Long timeout = parseTimeout(alertResult);
+ if (timeout == null) {
+ return alertResult;
+ }
+
String[] cmd = {"/bin/sh", "-c", scriptPath + ALERT_TITLE_OPTION + "'"
+ title + "'" + ALERT_CONTENT_OPTION
+ "'" + content + "'" + ALERT_USER_PARAMS_OPTION + "'" +
userParams + "'"};
- int exitCode = ProcessUtils.executeScript(cmd);
+ int exitCode = ProcessUtils.executeScript(timeout, cmd);
if (exitCode == 0) {
alertResult.setSuccess(true);
alertResult.setMessage("send script alert msg success");
return alertResult;
}
+ if (exitCode == ProcessUtils.EXECUTE_TIMEOUT_EXIT_CODE) {
+ alertResult.setMessage("send script alert msg error, script
execution timed out after " + timeout
+ + " seconds");
+ log.error("send script alert msg error, script execution timed out
after {} seconds", timeout);
+ return alertResult;
+ }
alertResult.setMessage("send script alert msg error,exitCode is " +
exitCode);
log.info("send script alert msg error,exitCode is {}", exitCode);
return alertResult;
}
+ private Long parseTimeout(AlertResult alertResult) {
+ if (StringUtils.isNotEmpty(timeoutConfig)) {
+ try {
+ return Long.parseLong(timeoutConfig);
+ } catch (NumberFormatException ex) {
+ log.warn("Invalid script timeout config value: '{}'",
timeoutConfig, ex);
+ alertResult.setMessage("script timeout config is invalid,
should be a number");
+ return null;
+ }
+ }
+ return (long) ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;
+ }
Review Comment:
`parseTimeout()` accepts any long value (including `0`/negative) and passes
it through to `ProcessUtils.executeScript()`, where `<= 0` disables the
timeout. To avoid accidental indefinite blocking, consider treating `timeout <=
0` as invalid (set an error message) or falling back to
`DEFAULT_SCRIPT_TIMEOUT`. Also consider using `isNotBlank()` + `trim()` so
whitespace-only values don’t get treated as an invalid number.
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java:
##########
@@ -106,18 +108,42 @@ private AlertResult executeShellScript(String title,
String content) {
return alertResult;
}
+ Long timeout = parseTimeout(alertResult);
+ if (timeout == null) {
+ return alertResult;
+ }
+
String[] cmd = {"/bin/sh", "-c", scriptPath + ALERT_TITLE_OPTION + "'"
+ title + "'" + ALERT_CONTENT_OPTION
+ "'" + content + "'" + ALERT_USER_PARAMS_OPTION + "'" +
userParams + "'"};
- int exitCode = ProcessUtils.executeScript(cmd);
+ int exitCode = ProcessUtils.executeScript(timeout, cmd);
if (exitCode == 0) {
alertResult.setSuccess(true);
alertResult.setMessage("send script alert msg success");
return alertResult;
}
+ if (exitCode == ProcessUtils.EXECUTE_TIMEOUT_EXIT_CODE) {
+ alertResult.setMessage("send script alert msg error, script
execution timed out after " + timeout
+ + " seconds");
+ log.error("send script alert msg error, script execution timed out
after {} seconds", timeout);
+ return alertResult;
+ }
Review Comment:
Using a fixed numeric exit code (`124`) to represent an internal timeout can
collide with real script exit codes (scripts can legitimately return 124). In
that case the sender would incorrectly report a timeout rather than the
script’s real failure reason. Consider using a sentinel exit code outside the
OS exit-code range (e.g., negative) or returning a richer result type (exitCode
+ timedOut flag) from `ProcessUtils` to avoid ambiguity.
##########
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java:
##########
@@ -17,23 +17,52 @@
package org.apache.dolphinscheduler.plugin.alert.script;
+import java.io.File;
+
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
/**
* ProcessUtilsTest
*/
public class ProcessUtilsTest {
- private static final String rootPath = System.getProperty("user.dir");
+ @Test
+ public void testExecuteScript() {
+ String javaBin = getJavaBin();
+ String[] cmd = {javaBin, "-cp", System.getProperty("java.class.path"),
+ ProcessUtilsTest.class.getName() + "$SimpleMain"};
+ int code = ProcessUtils.executeScript(60, cmd);
+ Assertions.assertNotEquals(ProcessUtils.EXECUTE_ERROR_EXIT_CODE, code);
+ }
- private static final String shellFilPath =
- rootPath +
"/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/script/shell/test.sh";
+ @Test
+ public void testExecuteScriptTimeout() {
+ String javaBin = getJavaBin();
+ String[] sleepCmd = {javaBin, "-cp",
System.getProperty("java.class.path"),
+ ProcessUtilsTest.class.getName() + "$SleepMain"};
+ int code = ProcessUtils.executeScript(2, sleepCmd);
+ Assertions.assertEquals(ProcessUtils.EXECUTE_TIMEOUT_EXIT_CODE, code);
+ }
Review Comment:
PR description says the timeout path “returns exit code -2”, but the
implementation/tests/docs use `EXECUTE_TIMEOUT_EXIT_CODE = 124`. Please update
the PR description (or the code) so the documented behavior and implementation
match, since this impacts how users interpret script exit codes.
--
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]
