Skip to content

DRb TimerIdConv cleanup #8399

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

Merged
merged 1 commit into from
May 3, 2016
Merged

DRb TimerIdConv cleanup #8399

merged 1 commit into from
May 3, 2016

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented May 2, 2016

https://bugzilla.redhat.com/show_bug.cgi?id=1321934

The TimerIdConv thread was not being terminated after a method ends.
The TimerIdConv thread sleeps for a hour before it GC's the objects

Here is an example of the thread count growth, but simplified outside of automate

require 'drb'
require 'drb/timeridconv'

100.times do
  # setup the drb server with a one second timer
  # creates a new thread that never dies
  timer = DRb::TimerIdConv.new(5)

  service = DRb.start_service("druby://127.0.0.1:0", nil, :idconv => timer)

  # ... some code that uses the DRb service

  # stop the drb server now that we're done with it
  service.stop_service

  # Below is ugly code that kills the timer thread.  Without the code below, the
  # thread stays alive forever.  This should have a public API.
  # It would be nice if DRb.start_service
  # accepted a :timer_idconv => 5 option that would create the timer thread itself
  # and was responsible for killing this thread when calling stop_service.

  # holder = timer.instance_variable_get(:@holder)
  # keeper = holder.instance_variable_get(:@keeper) if holder
  # keeper.kill if keeper
  puts Thread.list.length
  sleep 2
end

Thread count slowly rises:

2
3
4
5
6
7
8
9

Opened a ruby bug to see if we can fix this in ruby or through a cleaner API: https://bugs.ruby-lang.org/issues/12342 ?

@mkanoor
Copy link
Contributor Author

mkanoor commented May 2, 2016

@jrafanie @dmetzger57

Please review

@@ -238,6 +238,9 @@ def self.teardown_drb_for_ruby_method
DRb.stop_service
# Set the ID conv to nil so that the cache can be GC'ed
DRb.install_id_conv(nil)
holder = @@global_id_conv.instance_variable_get('@holder')
holder.instance_variable_get('@keeper').try(:kill) if holder
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking to see if there's a better way.

Copy link
Member

Choose a reason for hiding this comment

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

Please link to this for context and this so we can clean this up if they fix it in 2.3.2 or later. Please put a comment about what we're doing because it's not obvious at all.

@jrafanie
Copy link
Member

jrafanie commented May 3, 2016

https://bugzilla.redhat.com/show_bug.cgi?id=1321934

The TimerIdConv thread was not being terminated after a method ends.
The TimerIdConv thread sleeps for a hour before it GC's the objects
@jrafanie jrafanie merged commit 2c98312 into ManageIQ:master May 3, 2016
@jrafanie jrafanie added this to the Sprint 40 Ending May 9, 2016 milestone May 3, 2016
@miq-bot
Copy link
Member

miq-bot commented May 3, 2016

Checked commit mkanoor@1428c1e with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 🍪

chessbyte pushed a commit that referenced this pull request May 3, 2016
DRb TimerIdConv cleanup
(cherry picked from commit 2c98312)
jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants