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.

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

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Nov 17, 2021

I found the need for Timer.isCancelled() inside a long running timer that self-reschedules itself.

Example:

var timer = null

rule "Long running timer"
when
    System started
then
    timer = createTimer(now.plusSeconds(1), [|
        downloadFile() // do something that takes a long time here, e.g. a http get from a large source            
        timer.reschedule(now.plusSeconds(1))
    ])
end

rule "Cancel the timer"
when
    Item Switch1 received command ON
then
    timer.cancel()
end

When Switch1 received a command ON while downloadFile() is executing, the timer got cancelled, but the currently running task is still continuing, it will reschedule the timer and therefore the timer continues to run despite having been "cancelled".

In this case, the timer code can check with isCancelled() before calling reschedule as such:

rule "Long running timer"
when
    System started
then
    timer = createTimer(now.plusSeconds(1), [|
        downloadFile() // do something that takes a long time here, e.g. a http get from a large source
        if (!timer.isCancelled())  {
            timer.reschedule(now.plusSeconds(1))
        }
    ])
end

@jimtng jimtng requested a review from a team as a code owner November 17, 2021 09:35
@jimtng jimtng force-pushed the timer-iscancelled branch 3 times, most recently from 4a7b306 to d4cb926 Compare November 18, 2021 09:05
Signed-off-by: Jimmy Tanagra <[email protected]>
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@@ -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.

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.

@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Nov 23, 2021
@cweitkamp cweitkamp merged commit a4b737c into openhab:main Nov 23, 2021
@wborn wborn added this to the 3.2 milestone Dec 5, 2021
@jimtng jimtng deleted the timer-iscancelled branch February 16, 2022 21:13
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Jimmy Tanagra <[email protected]>
GitOrigin-RevId: a4b737c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants