Forum: Ruby flawed use of ConditionVariable

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
52a177e9dbd3e614825aabc4e45f8cd6?d=identicon&s=25 Mark Volkmann (Guest)
on 2006-01-10 17:08
(Received via mailing list)
Example code I've seen that uses a ConditionVariable seems to have a
timing flaw. For example, in Pickaxe 2, page 147 in the
Player/Customer example, the player thread does

  plays_pending.wait_while { playlist.empty? }

This assumes that ok_to_shutdown gets set and detected in the previous
line before wait_while is called. If the timing works out such that
ok_to_shutdown doesn't get set to true until after wait_while is
called, I believe it will hang forever.

Isn't it the case that the customer thread could finish and the player
thread could reach the call to wait_while before ok_to_shutdown gets
set to true?

I think what needs to happen is what I show in the following example.
Note how the wait_while checks whether the producer thread is alive
and how I do a condition.signal at the bottom when I know the producer
thread has terminated ... which causes wait_while to evaluate the
condition again.

The call to sleep at the end of the producer thread is critical to
illustrating the problem. If I take that out then I can get lucky with
the timing and don't need to test whether the producer thread is alive
in wait_while.

Am I off my rocker?  ;-)

require 'monitor'

#----------------------------------------------------------------------

class Widget
  attr_accessor :id

  def initialize(id)
    self.id = id
  end

  def to_s
    id
  end
end

#----------------------------------------------------------------------

bin = []
bin.extend MonitorMixin
condition = bin.new_cond # type is MonitorMixin::ConditionVariable

#----------------------------------------------------------------------

producer = Thread.new do
  widget_id = 0

  5.times do
    widget_id += 1
    widget = Widget.new(widget_id)
    puts "produced #{widget.to_s}"

    bin.synchronize do
      bin << widget
      condition.signal # there is a widget to consume
    end
  end

  # Delay termination of producer thread
  # to demonstrate need for another condition.signal.
  sleep 1
end

#----------------------------------------------------------------------

consumer = Thread.new do
  finished = false
  until (finished) do
    bin.synchronize do
      finished = (not producer.alive?) and bin.empty?
      break if finished

      # The condition will be tested once,
      # and then again every time condition.signal is called.
      condition.wait_while { producer.alive? and bin.empty? }

      widget = bin.shift # removes first Widget from bin
      puts "consumed #{widget.to_s}" if widget != nil
    end
  end
end

#----------------------------------------------------------------------

producer.join
bin.synchronize do
  # Make consumer retest condition after producer terminates.
  condition.signal
end
consumer.join
5befe95e6648daec3dd5728cd36602d0?d=identicon&s=25 Robert Klemme (Guest)
on 2006-01-10 18:39
(Received via mailing list)
Mark Volkmann wrote:
> Example code I've seen that uses a ConditionVariable seems to have a
> timing flaw. For example, in Pickaxe 2, page 147 in the
> Player/Customer example, the player thread does
>
>   plays_pending.wait_while { playlist.empty? }
>
> This assumes that ok_to_shutdown gets set and detected in the previous
> line before wait_while is called. If the timing works out such that
> ok_to_shutdown doesn't get set to true until after wait_while is
> called, I believe it will hang forever.

I think theoretically you are right but in practice this code will be
ok.
Reason: playing a song will take significantly longer than thread
"customer" needs to exit, main thread return from join and
ok_to_shutdown
set to true.  So while the player is still playing the flag will be set
to
true and once the player is finished playing the last song it will hit
the
line that checks the global shutdown flag.

In a Java VM this code would have the additional problem that access to
ok_to_shutdown is not thread safe.  The usual solution I choose for this
is that I put as many terminators (special objects) into the queue
(playlist here) that signal consumers that processing is over.  That way
no additional synchronization is needed and it's ensure that all
consumer
threads terminate.

> Isn't it the case that the customer thread could finish and the player
> thread could reach the call to wait_while before ok_to_shutdown gets
> set to true?
>
> I think what needs to happen is what I show in the following example.
> Note how the wait_while checks whether the producer thread is alive
> and how I do a condition.signal at the bottom when I know the producer
> thread has terminated ... which causes wait_while to evaluate the
> condition again.

I wouldn't check for threads being alive.  I'd rather synchronize
accesses
to ok_to_shutdown on playlist and notify when the state changes.  Then
the
check would have to be moved into the while loop in the player thread.

If I was to do this then I'd use class Queue.  That's thread safe and
you
easily get what you want:

require 'thread'

TERM = Object.new.freeze
QUEUE = Queue.new

# init stuff

consumer = Thread.new(QUEUE) do |queue|
  until TERM == (song = queue.deq)
    play(song)
  end
end

until (req = get_customer_request).nil?
  QUEUE.enc req
end
QUEUE.enc TERM

consumer.join


> The call to sleep at the end of the producer thread is critical to
> illustrating the problem. If I take that out then I can get lucky with
> the timing and don't need to test whether the producer thread is alive
> in wait_while.
>
> Am I off my rocker?  ;-)

No.  Good eyes!

Kind regards

    robert
This topic is locked and can not be replied to.