Copilot commented on code in PR #2475:
URL:
https://github.com/apache/shardingsphere-elasticjob/pull/2475#discussion_r2324796233
##########
test/native/src/test/java/org/apache/shardingsphere/elasticjob/test/natived/it/operation/JavaTest.java:
##########
@@ -305,4 +308,47 @@ void testShardingStatisticsAPI() {
});
job.shutdown();
}
+
+ @Test
+ void
testGivenOneShardingCountWhenShutdownThenTaskCanCaptureInterruptedException()
throws Exception {
+ testCaptureInterruptedException(1);
+ }
+
+ @Test
+ void
testGivenOneMoreShardingCountWhenShutdownThenTaskCanCaptureInterruptedException()
throws Exception {
+ testCaptureInterruptedException(2);
+ }
+
+ private void testCaptureInterruptedException(int shardingTotalCount)
throws Exception {
+ String jobName = "testTaskCaptureInterruptedTask";
+ final AtomicBoolean captured = new AtomicBoolean(false);
+ // start in 2 seconds
+ LocalTime tenSecondsLater = LocalTime.now().plusSeconds(2);
+ String cronExpression = String.format("%d %d %d * * ?",
tenSecondsLater.getSecond(), tenSecondsLater.getMinute(),
tenSecondsLater.getHour());
+ SimpleJob captureInterruptedTask = shardingContext -> {
+ int i = 0;
+ try {
+ while (true) {
+ if (Thread.currentThread().isInterrupted()) {
+ captured.set(true);
+ break;
+ }
+ i++;
+ Thread.sleep(100);
+ }
+ } catch (InterruptedException e) {
+ captured.set(true);
+ Thread.currentThread().interrupt();
+ }
+ };
Review Comment:
The variable `i` is incremented but never used. This creates unnecessary
computation in an infinite loop. Consider removing this unused counter.
##########
kernel/src/main/java/org/apache/shardingsphere/elasticjob/kernel/internal/schedule/LiteJob.java:
##########
@@ -19,20 +19,37 @@
import lombok.Setter;
import org.apache.shardingsphere.elasticjob.kernel.executor.ElasticJobExecutor;
+import org.quartz.InterruptableJob;
import org.quartz.Job;
import org.quartz.JobExecutionContext;
+import org.quartz.UnableToInterruptJobException;
+
+import java.util.Objects;
/**
* Lite job.
*/
@Setter
-public final class LiteJob implements Job {
+public final class LiteJob implements InterruptableJob {
private ElasticJobExecutor jobExecutor;
+ private Thread currentThread;
@Override
public void execute(final JobExecutionContext context) {
- jobExecutor.execute();
+ currentThread = Thread.currentThread();
+
+ try {
+ jobExecutor.execute();
+ } finally {
+ currentThread = null;
+ }
Review Comment:
There's a potential race condition where the `interrupt()` method could be
called after `currentThread` is set to null in the finally block but before the
method returns. Consider using a volatile field or synchronization to ensure
thread safety.
--
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: notifications-unsubscr...@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
