Reimplement TimerTask using promises #829
Conversation
adamruzicka
force-pushed
the
timer-task-2
branch
from
906068c to
a8be137
Compare
adamruzicka
force-pushed
the
timer-task-2
branch
from
a8be137 to
78b6188
Compare
|
The final changes are pretty straightforward, I just need to find time to actually implement them, hopefully sometime during this week. |
adamruzicka
force-pushed
the
timer-task-2
branch
from
e441148 to
fb43016
Compare
adamruzicka
force-pushed
the
timer-task-2
branch
from
604fe2c to
59d4c6a
Compare
| def shutdown | ||
| # TODO: Implement soft shutdown | ||
| kill | ||
| end |
There was a problem hiding this comment.
How exactly should soft shutdown behave?
There was a problem hiding this comment.
Hm I think there are actually 3 levels to consider.
- stop: finish task if running, do not schedule a new tasks
- shutdown: also cancel running task, do not schedule a new tasks
- kill: kill running tasks by killing it's thread, do not schedule a new tasks
What did the old implementation support?
For the new one I think all 3 make sense, could you implement them? Please free to experiment with the names or API for this, I am not sure myself what will be the best option.
There was a problem hiding this comment.
Not sure if we can do kill in a sensible way, will take a look what we can do
There was a problem hiding this comment.
The kill will always be dangerous. However, has to be there since the other methods can fail :/ It should be only used as a last resort when the previous methods failed to finish. If you run out of time just put raise NotImplemented there, or we could also meet for coffee and figure something together.
|
|
||
| @Channel = opts[:channel] | ||
|
|
||
| @Stopped = false |
There was a problem hiding this comment.
This one is not immutable. It got renamed by mistake.
| self.execution_interval = opts[:execution] || opts[:execution_interval] || EXECUTION_INTERVAL | ||
| self.timeout_interval = opts[:timeout] || opts[:timeout_interval] || TIMEOUT_INTERVAL | ||
|
|
||
| @Channel = opts[:channel] |
There was a problem hiding this comment.
I think we discussed some problems which could arise if we allow the channel to be shared between two timer-tasks. Like the arriving messages would not have clear sender. Is that right? It would be better if it would accept arguments for creating the channel, where new channel would be created on dup, or to add dup support on the channel and dup it as well in timer-task's dup.
There was a problem hiding this comment.
Right, how about we drop the reuse of channel in #dup. This way #duped timers won't have a channel, unless a new channel is provided when #dup is called?
There was a problem hiding this comment.
That would work but I think it would more surprising. Assuming that a duped timer-task is most often used in the same place/functional-context as the template than it would be expected the new timer-task to have a channel as the parent has.
| def shutdown | ||
| # TODO: Implement soft shutdown | ||
| kill | ||
| end |
There was a problem hiding this comment.
Hm I think there are actually 3 levels to consider.
- stop: finish task if running, do not schedule a new tasks
- shutdown: also cancel running task, do not schedule a new tasks
- kill: kill running tasks by killing it's thread, do not schedule a new tasks
What did the old implementation support?
For the new one I think all 3 make sense, could you implement them? Please free to experiment with the names or API for this, I am not sure myself what will be the best option.
|
Any news on this? Also, will this PR prevent the problem that TimerTask spins up a limitless number of threads in certain scenarios? |
|
Are people still interested in this PR? If so we can work on it. |
|
@chrisseaton well at this point it would really complicate things for me since I now maintain a version of timer task that fixes the thread leak issue: https://github.com/mhenrixon/sidekiq-unique-jobs/blob/main/lib/sidekiq_unique_jobs/timer_task.rb I still feel like the timer task shouldn't be leaking threads (and therefor memory) so yeah, please fix it! 😂 |
|
Since this is a draft PR, the author isn't looking to work on it any more, the branch has conflicts, and the work is incomplete, I'm closing it. Thanks anyway for it. If someone does want to merge it, we're open to that. |
TODO: