search

ruby-concurrency / concurrent-ruby

Modern concurrency tools including agents, futures, promises, thread pools, supervisors, and more. Inspired by Erlang, Clojure, Scala, Go, Java, JavaScript, and classic concurrency patterns.
https://ruby-concurrency.github.io/concurrent-ruby/
Other
5.68k stars 418 forks source link

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

Open bensheldon opened 1 year ago

bensheldon commented 1 year ago

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 commented 10 months ago

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 commented 7 months ago

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 commented 7 months ago

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.