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

Stop flush timer before closing queue #257

Merged
merged 1 commit into from
May 6, 2022

Conversation

abicky
Copy link
Contributor

@abicky abicky commented May 6, 2022

As Datadog::Sender#flush adds :flush command to the queue, the flush timer should be closed before the sender closes the queue to avoid ClosedQueueError.

@abicky abicky requested review from djmitche and remeh as code owners May 6, 2022 07:14
@@ -102,10 +102,11 @@ def start
# to close the sender nor trying to continue to `#add` more message
# into the sender.
def stop(join_worker: true)
Copy link
Contributor Author

@abicky abicky May 6, 2022

Choose a reason for hiding this comment

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

Without this change, the test fails as follows:

% bundle exec rspec spec/statsd/sender_spec.rb:116
Run options: include {:locations=>{"./spec/statsd/sender_spec.rb"=>[116]}}

Datadog::Statsd::Sender
  #stop
    when flush_interval is set
#<Thread:0x00007ff6e711b4e0@Statsd Timer /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/timer.rb:18 run> terminated with exception (report_on_exception is true):
/Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/sender.rb:43:in `push': queue closed (ClosedQueueError)
        from /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/sender.rb:43:in `flush'
        from /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/sender.rb:24:in `block in initialize'
        from /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/timer.rb:25:in `block (2 levels) in start'
        from /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/timer.rb:20:in `synchronize'
        from /Users/arabiki/ghq/src/github.com/DataDog/dogstatsd-ruby/lib/datadog/statsd/timer.rb:20:in `block in start'
      stops the worker thread and the flush timer thread (FAILED - 1)

Failures:

  1) Datadog::Statsd::Sender#stop when flush_interval is set stops the worker thread and the flush timer thread
     Failure/Error: current_message_queue.push(:flush)

     ClosedQueueError:
       queue closed
     # ./lib/datadog/statsd/sender.rb:43:in `push'
     # ./lib/datadog/statsd/sender.rb:43:in `flush'
     # ./lib/datadog/statsd/sender.rb:24:in `block in initialize'
     # ./lib/datadog/statsd/timer.rb:25:in `block (2 levels) in start'
     # ./lib/datadog/statsd/timer.rb:20:in `synchronize'
     # ./lib/datadog/statsd/timer.rb:20:in `block in start'

Finished in 0.01402 seconds (files took 0.4371 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/statsd/sender_spec.rb:117 # Datadog::Statsd::Sender#stop when flush_interval is set stops the worker thread and the flush timer thread

@abicky abicky force-pushed the fix-queue-closed branch 2 times, most recently from fb8532b to 91a6393 Compare May 6, 2022 07:33
As `Datadog::Sender#flush` adds `:flush` command to the queue, the
flush timer should be closed before the sender closes the queue to
avoid ClosedQueueError.
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

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

Makes total sense, thanks for your contribution!

@remeh remeh merged commit 7625ec6 into DataDog:master May 6, 2022
@abicky abicky deleted the fix-queue-closed branch May 15, 2022 08:53
@abicky
Copy link
Contributor Author

abicky commented May 20, 2022

@remeh Thank you for merging the PR, and I'm sorry I didn't notice the bug when I implemented #231.
Do you have any plan to release a new version? I'd like to use the version including this bug fix.

@aniruddhb
Copy link

aniruddhb commented May 28, 2022

Echo'oing @abicky , it would be awesome to get this fix released as part of the gem 😃 Appreciate all the work that goes into this piece of software.

@remeh
Copy link
Contributor

remeh commented May 31, 2022

@abicky @aniruddhb I'll work on a new release this week 👍

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

Successfully merging this pull request may close these issues.

3 participants