Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

GirlFriday::Batch exhibits odd behavior #54

jluxenberg opened this Issue May 29, 2012 · 4 comments


None yet
4 participants

I have the following test case:

require 'set'
require 'pp'
require 'girl_friday'

def assert(cond)
  raise RuntimeException.new("failure") unless cond

batch = GirlFriday::Batch.new(nil, :size => 5) do |msg|

0.upto(100) do |i|
  batch.push({:n => i})

result = batch.results
assert(Set.new(result) == Set.new(0..100))

When I run it, it blocks for 30 seconds at the end and completes successfully:

$ bundle exec ruby -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [i686-linux]
$ bundle list
Gems included by the bundle:
  * activemodel (3.2.3)
  * activerecord (3.2.3)
  * activesupport (3.2.3)
  * arel (3.0.2)
  * builder (3.0.0)
  * bundler (1.1.4)
  * connection_pool (0.1.0)
  * girl_friday (0.9.7)
  * i18n (0.6.0)
  * multi_json (1.3.5)
  * mysql (2.8.1)
  * mysql2 (0.3.11)
  * sqlite3 (1.3.6)
  * tzinfo (0.3.33)
$ time bundle exec ruby friday_testcase.rb

real    0m30.432s
user    0m0.396s
sys 0m0.036s

However, if I include a "puts 'foo'" at the end of the test case, it works fine (competes in under a second).

lexmag commented Jun 11, 2012

Problem is in GirlFriday.shutdown! method:

# girl_friday.rb
def self.shutdown!(timeout=30)
  m.synchronize do
    var.wait(m, timeout) if count != 0

We get there +30 seconds. YARV only.

lexmag commented Jun 12, 2012

There is more deeper problem with GC in YARV. http://bugs.ruby-lang.org/issues/4168
At_exit callback still have alive weakref for shutdowned queue.


xaviershay commented Jul 19, 2012

I am able to replicate this.


mperham commented Jul 20, 2012

Yes, shutdown has a race condition in it. The test suite fails intermittently in several places.

I don't use girl_friday anymore (sidekiq takes 90% of my OSS time these days) so I can't justify spending the time to fix it. If it was up to me, I would remove all the weakref stuff and have the user explicitly register all long-lived queues to be shutdown at process exit but that's a major API change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment