TimerTask: kill the execution when it exceeds the task timeout #749
Conversation
rubemz
force-pushed
the
fix/timer-task-leaking
branch
from
d1f135c to
c772e76
Compare
…d timer_task timeout specs Before this commit when a TimerTask task exceeded the timeout the job kept running. That could lead to thread leaking. Related to ruby-concurrency#639 This PR changes the TimerTask executor to be a SingleThreadExecutor. So whenever the task doesn't finish in time, the execution thread is killed (in this case the pool is killed) The spec ensures that this new behavior is working correctly. It fails on master. If the main job isn't killed after the timeout, the latch.wait(2) would raise, since the main task sleeps for 5 seconds. Closes ruby-concurrency#639
rubemz
force-pushed
the
fix/timer-task-leaking
branch
from
c772e76 to
5a65b76
Compare
lib/concurrent/timer_task.rb
Outdated
| @@ -325,6 +339,10 @@ def execute_task(completion) | |||
| def timeout_task(completion) | |||
| return unless @running.true? | |||
| if completion.try? | |||
| @executor.kill | |||
| @executor.wait_for_termination | |||
| @executor = Concurrent::RubySingleThreadExecutor.new() | |||
|
Thanks for the fix, I'll have a look in couple of days. |
ghost
added
the
|
Hi, sorry I did not forgot about your PR. I needed to get out 1.1.0.pre1 for testing. I'll review the PR soon. |
| @@ -280,7 +280,8 @@ def ns_initialize(opts, &task) | |||
| self.execution_interval = opts[:execution] || opts[:execution_interval] || EXECUTION_INTERVAL | |||
| self.timeout_interval = opts[:timeout] || opts[:timeout_interval] || TIMEOUT_INTERVAL | |||
| @run_now = opts[:now] || opts[:run_now] | |||
| @executor = Concurrent::SafeTaskExecutor.new(task) | |||
| @task = task | |||
| @executor = Concurrent::RubySingleThreadExecutor.new | |||
There was a problem hiding this comment.
This needs a corresponding change to the require statements at top of this file (remove concurrent/executor/safe_task_executor, add concurrent/executor/ruby_single_thread_executor)
|
Sorry still did not find time to do a proper review. There is a problem with killing threads, if it's killed while running e.g. ensure block it may corrupt data and other unpleasant things (more info e.g. https://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/). I need to give this a more careful thought. |
|
We removed timer timeouts in #926 given their problems. We're open to discussing more options. |
Before this commit when a TimerTask task exceeded the timeout the job kept running. That could lead to thread leaking. Related to #639
This PR changes the TimerTask executor to be a SingleThreadExecutor. So whenever the task doesn't finish in time, the execution thread is killed (in this case the pool is killed).
The spec ensures that this new behavior is working correctly. It fails on master. If the main job isn't killed after the timeout, the
latch.wait(2)would raise, since the timer task sleeps for 5 seconds.If you want to see the leak by yourself take a look on this https://github.com/rubemz/concurrent-ruby-leak, which is pretty much a copy from #639 (comment). @shun0309 thanks for that code.
Closes #639