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

Be sure to detach one-shot timer watcher #1178

Merged
merged 3 commits into from
Aug 24, 2016

Conversation

okkez
Copy link
Contributor

@okkez okkez commented Aug 22, 2016

@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.

Because,

> 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.
@okkez
Copy link
Contributor Author

okkez commented Aug 23, 2016

I've pushed change for comment @tagomoris

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.

This makes test faster and stable.
@tagomoris
Copy link
Member

LGTM.

@tagomoris tagomoris merged commit 12718a2 into fluent:master Aug 24, 2016
@okkez okkez deleted the detach-oneshot-timer branch August 24, 2016 02:07
@okkez
Copy link
Contributor Author

okkez commented Aug 24, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants