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.

Sign up for GitHub

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

Be sure to detach one-shot timer watcher #1178

Merged
merged 3 commits into from
Aug 24, 2016
Merged

Be sure to detach one-shot timer watcher #1178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4f0332
Be sure to detach one-shot timer watcher
okkez Aug 22, 2016
80ab617
Stop using Coolio::TimerWatcher instance variable
okkez Aug 22, 2016
4491517
Use busy loop instead of plain sleep
okkez Aug 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/fluent/plugin_helper/timer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,12 @@ def on_timer
rescue => e
@log.error "Unexpected error raised. Stopping the timer.", title: @title, error: e
@log.error_backtrace
self.detach
detach
@log.error "Timer detached.", title: @title
ensure
if attached?
detach unless @repeating
Copy link
Member

Choose a reason for hiding this comment

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

Is @repeating an instance variable of TimerWatcher?
I think it's too internal to use it, and it looks too fragile.
We should store repeat into our own instance variable in #initialize and use it.

end
end
end
end
Expand Down
26 changes: 26 additions & 0 deletions test/plugin_helper/test_timer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,30 @@ class Dummy < Fluent::Plugin::TestBase

d1.shutdown; d1.close; d1.terminate
end

test 'can run at once' do
d1 = Dummy.new
d1.configure(config_element())
assert !d1.timer_running?
d1.start
assert d1.timer_running?

counter = 0
d1.timer_execute(:test, 1, repeat: false) do
counter += 1
end

watchers = d1._event_loop.watchers.reject {|w| w.is_a?(Fluent::PluginHelper::EventLoop::DefaultWatcher) }
assert_equal(1, watchers.size)
assert(watchers.first.attached?)

sleep 3
Copy link
Member

Choose a reason for hiding this comment

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

This sleep 3 makes tests slow and unstable.
The better way to make it well is like this (not tested code):

waiting_assertion = true
waiting_timer = true

counter = 0
d1.timer_execute(:test, 1, repeat: false) do
  sleep 0.1 while waiting_assertion
  counter += 1
  waiting_timer = false
end

# assertions about watchers here
waiting_assertion = false

sleep 0.1 while waiting_timer

assert_equal(1, counter)
# rest of assertions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for sample code.
I will fix test code later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a change for this comment.


assert_equal(1, counter)
assert_false(watchers.first.attached?)
watchers = d1._event_loop.watchers.reject {|w| w.is_a?(Fluent::PluginHelper::EventLoop::DefaultWatcher) }
assert_equal(0, watchers.size)

d1.shutdown; d1.close; d1.terminate
end
end