igrigorik/agent

Agent is an attempt at modelling Go-like concurrency, in Ruby

agent's Issues

Race condition in selector

There is a race condition in the Kernel#select method beween the yield and the call to Selector#select. More specifically, the race condition is between the check to see if a channel is immediately ready for use and when callbacks are registered. If a channel becomes available for reading or writing in between these then the callbacks may never fire despite the conditions being met at the time of the call to Selector#select.

In order to fix this race condition with minimum code changes you'd need to freeze the channels as they are added in the case method such that they're frozen between the check to see if it's immediately available and until the callbacks are added. There could be significant delay there, however, so I don't think freezing them there is a good idea.

You would still need to freeze them in order to synchronize the channels either way, I think. A better way to handle this might be to have the case method preserve the order that the selector callbacks are added and not check whether the channel is immediately available. We could then freeze the channels in the select method check them for immediate availability in the order that now has been preserved in the case, handle the default case if it's there, add the notification callbacks to the channels, then unfreeze the channels.

I'm not sure how else we could eliminate that race condition, but I'm open to ideas.

Notification channel may block channels indefinitely

In Selector#select, a Notification channel is created. This channel could be written once per channel given in the select statement. Only the first element in the channel is ever read, and every subsequent attempt to write to it (except the second one, which will succeed since the channel is emptied after the first element is read) will block indefinitely. This is a very real possibility if the Thread switches between the first receive on the Notification channel and when all callbacks are finished being removed from the channels.

I think the solution here is a channel without a max, or a channel which throws away every write but the first one. This wouldn't require any locks at all, keeping things fast.

Get rid of channel registry

The global registry for channels is a potential source of memory leaks. We could potentially special-case channels and dup them rather than marshal them so that we don't lose references to them.

examples

Hello,
Do anybody know how to rewrite the following thread examples with agent:
http://shootout.alioth.debian.org/u64q/program.php?test=spectralnorm&lang=yarv&id=3
http://shootout.alioth.debian.org/u64q/program.php?test=regexdna&lang=yarv&id=3
http://shootout.alioth.debian.org/u64q/program.php?test=knucleotide&lang=yarv&id=1
http://shootout.alioth.debian.org/u64q/program.php?test=mandelbrot&lang=yarv&id=2
http://shootout.alioth.debian.org/u64q/program.php?test=fannkuchredux&lang=yarv&id=2

Is it also possible to use agent with http://rubini.us/ ?

Thank you in advance.

Channels don't behave like go's channels

Receiving on a closed channel in agent will raise an error. In go it will return immediately with the zero value for the object's type and false (in the case of a multi-value receive).

http://golang.org/ref/spec#Close

We should fix this, but it's a pretty major change and so we should at least bump a minor version.

Memory leak w/ channel names

Channel names are required to be symbols and in Selector the channels for timeouts and notifications are UUIDs. This means that each time select is used it will add two symbols to ruby's symbol table.

Also, since channel names are required, users may resort to doing something similar when they don't care about the name but want to make sure that the Queue behind the scenes isn't shared between unrelated channels. I could see myself doing it.

Maybe a better way to handle storage of the memory queues would be to use a hash instead of class variables? You could then also auto-generate the name (as a string) if one is not provided, which would be nice as well.

upgraded to 1.9.1

/home/tyler/.rvm/gems/ruby-1.9.1-p378/gems/agent-0.1.0/lib/agent/transport/queue.rb:19:in initialize': uninitialized constant Agent::Transport::Queue::ConditionVariable (NameError) from /home/tyler/.rvm/gems/ruby-1.9.1-p378/gems/agent-0.1.0/lib/agent/channel.rb:20:innew'
from /home/tyler/.rvm/gems/ruby-1.9.1-p378/gems/agent-0.1.0/lib/agent/channel.rb:20:in initialize' from myagent.rb:4:innew'
from myagent.rb:4:in `

'

Agent overrides Kernel.select

Kernel already has a select method. Not sure how this one went so long without being noticed. I've been staring at this thing for a long time and it only just now occurred to me.

Anyways, overriding Kernel.select is a very bad thing, imho.

Don't marshal certain objects

Some objects are singletons or are virtually immutable, so there's no need to marshal them. We could potentially let users specify their own classes to ignore or specify how to handle them.

Deadlock?

Hi, I'm not sure if I'm doing this wrong, but the following code prints waiting forever and never exits. I'm guessing there is a deadlock somewhere, but I'm not sure.

require 'agent'

channel = channel!(Integer)

def read_channel
  channel.receive
end

go! do
  puts read_channel
  channel.close
end

go! do
  channel.send(1)
end

while not channel.closed? do
  sleep 0.1
  puts 'waiting'
end

puts 'done'

If I replace the function with a lambda instead, then it works fine, done is printed and the program exits. Do you know why this might be happening? Thank you for making this gem!

require 'agent'

channel = channel!(Integer)

read_channel = lambda do
  channel.receive
end

go! do
  puts read_channel.call
  channel.close
end

go! do
  channel.send(1)
end

while not channel.closed? do
  sleep 0.1
  puts 'waiting'
end

puts 'done'

More race conditions in the selector

If you check out go's select statement, you'll notice that receiving and sending actual values are part of the case conditions. I think this is so they can make it atomically receive from or send to one and only one channel.

Agent's API looks like this, presently:

old_value = 3

select do |s|
  s.case(cr, :receive) { |c| new_value = c.receive }
  s.case(cw, :send)    { |c| c.send old_value  }
end

If the cr channel becomes available for receive first, it will have its block called. A race condition is introduced, however, because the thread could switch context away from the block at any time and another thread could call receive on the channel and steal its value. If this happens and we switch back to the block, we could get stuck at the c.receive call indefinitely.

The only way to prevent this from occurring with the current syntax is to put a lock on the channel for the entire block's duration so that its state is preserved (the state of being able to send to or receive from the channel). That's not desirable at all, especially since this could become circular and you could end up with deadlocks.

It's becoming much clearer why go implemented the select this way.

I propose a new syntax that is more similar to go's and is easier to prevent race conditions with:

old_value = 3

select do |s|
  s.case(cr, :receive) { |new_value| puts "The new value is that was received is #{new_value}" }
  s.case(cw, :send, old_value)    { puts "Old value was sent via the channel at this point" }
end

I think this is much closer to the way that go functions, will prevent undesirable behavior as I described above, and gives us the best chance of making this library truly thread safe.

v0.11.1 / v0.12.0

Hello guys,

Do you think it would be appropriate to release a new version of the gem after the last PRs that fixed the concurrent behavior?

Given that the project is more or less stable and not under heavy development, I think it would make sense.

Thanks

not getting love

go.rb:3: syntax error, unexpected ':', expecting ')'
c = Agent::Channel.new(name: 'incr', type: Integer)
^
go.rb:3: syntax error, unexpected ',', expecting $end
c = Agent::Channel.new(name: 'incr', type: Integer)

Interesting article...

You got some nice exposure from this article:
http://www.sitepoint.com/agent-go-like-concurrency-ruby

One thing that bothered me from this, and I'm not sure about what you're doing yet as this is the first time I heard of your library and thought I'd just ask the question since there's potential misinformation going on: since JRuby offers concurrent parallelism, can't Agent likewise leverage this in order to give us true concurrent parallelism in our apps? We have been doing some work in Go, but keep trying to shift back to ruby as we prefer the full stack ruby environment using it with JRuby on our servers, Opal to replace JS, and then RubyMotion for mobile app (iOS and now Android). Having Agent offer true parallel processing facilities would be fantastic.

Not normally

I'm not sure this code, this is correct?

#! /usr/bin/env ruby

require 'agent'
require 'time'
require 'nokogiri'
require 'open-uri'

t = Time.now
DIR = t.strftime('%Y-%m')
Dir.mkdir(DIR) unless File.exists?(DIR)

# go routines
go! do

   date = dateString
   filename = date + '.md'

   createMarkDown(date, filename)

   scrape("ruby", filename)
   scrape("go", filename)
   scrape("python", filename)
   scrape("php", filename)
   scrape("javascript", filename)
   
   sleep(24.hours)
   # sleep(1.days)
   # or using time
   # sleep(Time.parse("22:00:00") - Time.now)
end

def scrape(language, filename)
  target = File.open("#{DIR}/#{filename}", "a")

  target.write("\n####")

  target.write(language)

  target.write("\n\n")

  uri = "https://github.com/trending?l=#{language}"

  doc = Nokogiri::HTML(open(uri))
  rows = doc.css('ol.repo-list li')

  rows.each do |row|
  
    hrefs = row.css("h3 a").map{ |a| 
      a['href']
    }.compact.uniq
   
    hrefs.each do |href|
      remote_url = "https://github.com"+href
      title = row.css('h3 a').text()
      description = row.css('p').text()
      puts "Fetching #{remote_url}..."
      target.write("* [" + title.strip! + "](" + remote_url + "): " + description.strip! + "\n")
    end # done: hrefs.each

  end # done: rows.each
  target.close
end

def dateString
  t = Time.now
  return t.strftime('%Y-%m-%d')
end

def createMarkDown(date, filename)
  s = "###" + date + "\n"
  local_fname = "#{DIR}/#{filename}"
  File.open(local_fname, 'w'){ |file|
    file.write(s)
  }
end

loop do
end