Skip to content
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.

Sign up for GitHub

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 Timer.isCancelled() method #2570

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Add Timer.isCancelled() method #2570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...ipt/src.moved/test/java/org/openhab/core/model/script/internal/actions/TimerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,53 +52,64 @@ public void setUp() {
public void testTimerIsActiveAndCancel() {
assertThat(subject.isActive(), is(true));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));

subject.cancel();
assertThat(subject.isActive(), is(false));
assertThat(subject.hasTerminated(), is(true));
assertThat(subject.isCancelled(), is(true));

subject.reschedule(ZonedDateTime.now().plusSeconds(DEFAULT_TIMEOUT_SECONDS));
assertThat(subject.isActive(), is(true));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));
}

@Test
public void testTimerIsActiveAndTerminate() throws InterruptedException {
assertThat(subject.isActive(), is(true));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));

Thread.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_TIMEOUT_SECONDS + DEFAULT_RUNTIME_SECONDS + 1));
assertThat(subject.isActive(), is(false));
assertThat(subject.hasTerminated(), is(true));
assertThat(subject.isCancelled(), is(false));
}

@Test
public void testTimerHasTerminatedAndReschedule() throws InterruptedException {
Thread.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_TIMEOUT_SECONDS + DEFAULT_RUNTIME_SECONDS + 1));
assertThat(subject.isActive(), is(false));
assertThat(subject.hasTerminated(), is(true));
assertThat(subject.isCancelled(), is(false));

subject.reschedule(ZonedDateTime.now().plusSeconds(DEFAULT_TIMEOUT_SECONDS));
assertThat(subject.isActive(), is(true));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));

Thread.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_TIMEOUT_SECONDS + DEFAULT_RUNTIME_SECONDS + 1));
assertThat(subject.isActive(), is(false));
assertThat(subject.hasTerminated(), is(true));
assertThat(subject.isCancelled(), is(false));
}

@Test
public void testTimerIsRunning() throws InterruptedException {
assertThat(subject.isRunning(), is(false));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));

Thread.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_TIMEOUT_SECONDS) + 500);
assertThat(subject.isRunning(), is(true));
assertThat(subject.hasTerminated(), is(false));
assertThat(subject.isCancelled(), is(false));

Thread.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_RUNTIME_SECONDS + 1));
assertThat(subject.isRunning(), is(false));
assertThat(subject.hasTerminated(), is(true));
assertThat(subject.isCancelled(), is(false));
}

private Timer createTimer(ZonedDateTime instant, SchedulerRunnable runnable) {
Expand Down
7 changes: 7 additions & 0 deletions bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Timer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ public interface Timer {
*/
public boolean isActive();

/**
* Determines whether the timer has been cancelled
*
* @return true, if the timer has been cancelled, false otherwise
*/
public boolean isCancelled();

/**
* Determines whether the scheduled code is currently executed.
*
Expand Down
13 changes: 7 additions & 6 deletions ...enhab.core.model.script/src/org/openhab/core/model/script/internal/actions/TimerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ public class TimerImpl implements Timer {
private final SchedulerRunnable runnable;
private ScheduledCompletableFuture<Object> future;

private boolean cancelled;

public TimerImpl(Scheduler scheduler, ZonedDateTime startTime, SchedulerRunnable runnable) {
this.scheduler = scheduler;
this.startTime = startTime;
Expand All @@ -48,26 +46,29 @@ public TimerImpl(Scheduler scheduler, ZonedDateTime startTime, SchedulerRunnable

@Override
public boolean cancel() {
cancelled = true;
return future.cancel(true);
}

@Override
public boolean reschedule(ZonedDateTime newTime) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to your contribution but I am wondering if we should synchronize this method to have a thread safe implementation? Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to be thread safe already https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/CompletableFuture.html#cancel(boolean)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I am talking about is a small race condition on the future property of this implementation when calling the reschedule(...) method because we first cancel the existing future and afterwards assign a new CompletableFuture to it.

future.cancel(false);
cancelled = false;
future = scheduler.schedule(runnable, newTime.toInstant());
return true;
}

@Override
public ZonedDateTime getExecutionTime() {
return cancelled ? null : ZonedDateTime.now().plusNanos(future.getDelay(TimeUnit.NANOSECONDS));
return future.isCancelled() ? null : ZonedDateTime.now().plusNanos(future.getDelay(TimeUnit.NANOSECONDS));
}

@Override
public boolean isActive() {
return !future.isDone() && !cancelled;
return !future.isDone();
}

@Override
public boolean isCancelled() {
return future.isCancelled();
}

@Override
Expand Down