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

Allow TimerTask to be safely restarted after shutdown and avoid duplicate tasks #1001

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bensheldon
Copy link
Contributor

@bensheldon bensheldon commented May 29, 2023

Fixes #672 and #662.

Creates a new @running object on shutdown while the ScheduledTask is checking the previously set @running object which will remain false.

Note: I don't like my testing strategy but couldn't think of a better one.

@ioquatix
Copy link
Contributor

I have several concerns about the implementation of TimerTask and the usage of @running. While this PR on the surface looks okay to me, @matthewd do you mind taking a closer look? I feel like we need to make sure the semantics of execute and shutdown are clear.

@matthewd
Copy link
Contributor

matthewd commented Mar 1, 2024

Yeah, I think I agree there are some semantic questions here.

Particularly given the explicitly documented "run-then-sleep-N loop" implementation (as opposed to "run every N"), I think it's reasonable to expect that only one instance of the block will be running at any given time. (Notwithstanding the internal use of the completion event, which almost feels like a nod toward at-least-once behaviour, but looks to me like it actually became redundant when the timeout behaviour [which involved two racing tasks] was removed.)

The linked issues are clearly violations of that principle, and this PR reduces the circumstances where that can happen... but an execute; shutdown; execute could still leave two copies overlapping if the first run starts just before the shutdown.

Setting aside whether it's a complete fix, in terms of whether this PR is safe in isolation: I'm not certain of the semantics of an unsynchronized read of an instance variable that's potentially being written by another (synchronized) thread. Without going back to research it properly, the local implementation of execution_interval seems to support my vague feeling that it may be problematic.

On implementation, I note that ScheduledTask has a potentially-relevant cancel method. If we have to switch to an AtomicReference anyway, perhaps we can just remember the upcoming ScheduledTask and then cancel it directly when necessary?

@bensheldon
Copy link
Contributor Author

bensheldon commented Mar 1, 2024

I force-pushed over the change, but task.cancel if task required being called outside of synchronization and thus has the same danger of unsynchronized read/write.

In consultation with @matthewd, I I'm now using an AtomicFixnum to store and check the "age" of the TimerTask (incremented when started/restaretd) to try to avoid any variable reassignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shutdown and execute a timertask results in double runs
3 participants