Concurrency bug in ControlTower
Apologies reporting this here; I'm not sure where I should report bugs in ControlTower. I believe there's a bug in ControlTower around line 34: def open @status = :open while (@status == :open) connection = @socket.accept # << here @request_queue.async(@request_group) do The "connection" local variable is used in the async block below to parse the request and send the response. Unless MacRuby's blocks behave differently than regular Ruby, this variable is shared across all activations of that block. As a result, it's possible that two concurrent requests will end up using each others' connection, and usually just blowing up as a result. The fix I've come up with is to wrap the @request_queue.async call in a tap call: def open @status = :open while (@status == :open) conn = @socket.accept conn.tap do |connection| @request_queue.async(@request_group) do Since async can't accept any explicitly-passed state, this seems like the safest way to ensure the connection reference is not shared by separate invocations. I'm not sure if it's possible in GCD, but having async take an optional argument with explicitly-passed state might also be a good way to fix this. - Charlie
Hey Charles, Sure, this is as good a place as any to report issues on ControlTower. There's also a component for ControlTower on the MacRuby trac site, but I guess we don't really call that out on the web site. Regarding the potential bug...well...where to begin? So, yes, MacRuby blocks have the same semantics as regular Ruby blocks, *except* when they are dispatched through GCD. In that case, ivars are shared, but local variable get copied. I'm sure Laurent can explain why better than I, but it has to do with the semantics of libdispatch and the uncertainty inherent in when a dispatched block or function will execute (i.e. if local variables were not copied during dispatch, they might go out of scope and be collected before GCD ever gets around to running the code). To illustrate how this impacts async semantics wrt ruby, here's a sample done with MacRuby/GCD and canonical Ruby: Earth: ~/Desktop > cat macfruit.rb fruit_sellers = Dispatch::Group.new offerings = %w|apples oranges bananas pinaples coconuts| idx = 0 while(idx < 5) fruit = offerings[idx] Dispatch::Queue.concurrent.async(fruit_sellers) do sleep 5-idx puts "I sell #{fruit}, fresh as can be. Come and get some!" end idx += 1 end fruit_sellers.wait puts "What would you like?" Earth: ~/Desktop > macruby macfruit.rb I sell coconuts, fresh as can be. Come and get some! I sell pinaples, fresh as can be. Come and get some! I sell bananas, fresh as can be. Come and get some! I sell oranges, fresh as can be. Come and get some! I sell apples, fresh as can be. Come and get some! What would you like? Earth: ~/Desktop > macruby macfruit.rb I sell coconuts, fresh as can be. Come and get some! I sell pinaples, fresh as can be. Come and get some! I sell bananas, fresh as can be. Come and get some! I sell oranges, fresh as can be. Come and get some! I sell apples, fresh as can be. Come and get some! What would you like? Earth: ~/Desktop > cat mrifruit.rb fruit_sellers = [] offerings = %w|apples oranges bananas pinaples coconuts| idx = 0 while(idx < 5) fruit = offerings[idx] fruit_sellers << Thread.new do sleep 5-idx puts "I sell #{fruit}, fresh as can be. Come and get some!" end idx += 1 end fruit_sellers.each(&:join) puts "What would you like?" Earth: ~/Desktop > ruby mrifruit.rb I sell coconuts, fresh as can be. Come and get some! I sell coconuts, fresh as can be. Come and get some! I sell coconuts, fresh as can be. Come and get some! I sell coconuts, fresh as can be. Come and get some! I sell coconuts, fresh as can be. Come and get some! What would you like? In other words, not a bug in MacRuby...also, this code is scheduled for a rewrite that will turn it into a bit more of a continuation-passing style of handling the connections. I don't think tap is really the solution we are looking for here, since after dispatching, the main #accept loop doesn't really care about that connection (other than that, at some point, it is responsibly closed). Cheers, Josh On Sat, Jan 22, 2011 at 2:14 AM, Charles Oliver Nutter <headius@headius.com>wrote:
Apologies reporting this here; I'm not sure where I should report bugs in ControlTower.
I believe there's a bug in ControlTower around line 34:
def open @status = :open while (@status == :open) connection = @socket.accept # << here
@request_queue.async(@request_group) do
The "connection" local variable is used in the async block below to parse the request and send the response. Unless MacRuby's blocks behave differently than regular Ruby, this variable is shared across all activations of that block. As a result, it's possible that two concurrent requests will end up using each others' connection, and usually just blowing up as a result.
The fix I've come up with is to wrap the @request_queue.async call in a tap call:
def open @status = :open while (@status == :open) conn = @socket.accept
conn.tap do |connection| @request_queue.async(@request_group) do
Since async can't accept any explicitly-passed state, this seems like the safest way to ensure the connection reference is not shared by separate invocations. I'm not sure if it's possible in GCD, but having async take an optional argument with explicitly-passed state might also be a good way to fix this.
- Charlie _______________________________________________ MacRuby-devel mailing list MacRuby-devel@lists.macosforge.org http://lists.macosforge.org/mailman/listinfo.cgi/macruby-devel
On Mon, Jan 24, 2011 at 11:13 AM, Joshua Ballanco <jballanc@gmail.com> wrote:
Regarding the potential bug...well...where to begin? So, yes, MacRuby blocks have the same semantics as regular Ruby blocks, *except* when they are dispatched through GCD. In that case, ivars are shared, but local variable get copied. I'm sure Laurent can explain why better than I, but it has to do with the semantics of libdispatch and the uncertainty inherent in when a dispatched block or function will execute (i.e. if local variables were not copied during dispatch, they might go out of scope and be collected before GCD ever gets around to running the code).
Wow, that's very surprising. I'm not sure I agree with bending Ruby semantics so drastically, even to help concurrency. Or at least, I'd expect other threaded scenarios to be consistent: ~ ➔ ruby -ve "a = 0; Thread.new { a += 1 }.join; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 1 ~ ➔ ruby -ve "a = 0; q = Dispatch::Queue.concurrent; q.sync {a += 1}; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 0 The implicitness in being able to mutate the surrounding scope is certainly problematic. This is one reason Java's anonymous inner classes require that referenced variables be declared final...to indicate they can't be mutated by the body of the anonymous inner class's methods. The result is that people end up using one-element arrays and the like, but people find ways around anything. So I suppose this applies to anything in the surrounding scope, including visibility, $~, and so on?
To illustrate how this impacts async semantics wrt ruby, here's a sample done with MacRuby/GCD and canonical Ruby:
I'll see how this might be implemented in JRuby.
In other words, not a bug in MacRuby...also, this code is scheduled for a rewrite that will turn it into a bit more of a continuation-passing style of handling the connections. I don't think tap is really the solution we are looking for here, since after dispatching, the main #accept loop doesn't really care about that connection (other than that, at some point, it is responsibly closed).
tap would be a clean way to explicitly isolate each loop's environment, if you weren't already doing that. - Charlie
On Jan 26, 2011, at 12:49 AM, Charles Oliver Nutter wrote:
On Mon, Jan 24, 2011 at 11:13 AM, Joshua Ballanco <jballanc@gmail.com> wrote:
Regarding the potential bug...well...where to begin? So, yes, MacRuby blocks have the same semantics as regular Ruby blocks, *except* when they are dispatched through GCD. In that case, ivars are shared, but local variable get copied. I'm sure Laurent can explain why better than I, but it has to do with the semantics of libdispatch and the uncertainty inherent in when a dispatched block or function will execute (i.e. if local variables were not copied during dispatch, they might go out of scope and be collected before GCD ever gets around to running the code).
Wow, that's very surprising. I'm not sure I agree with bending Ruby semantics so drastically, even to help concurrency. Or at least, I'd expect other threaded scenarios to be consistent:
~ ➔ ruby -ve "a = 0; Thread.new { a += 1 }.join; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 1
~ ➔ ruby -ve "a = 0; q = Dispatch::Queue.concurrent; q.sync {a += 1}; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 0
The implicitness in being able to mutate the surrounding scope is certainly problematic. This is one reason Java's anonymous inner classes require that referenced variables be declared final...to indicate they can't be mutated by the body of the anonymous inner class's methods.
The result is that people end up using one-element arrays and the like, but people find ways around anything.
So I suppose this applies to anything in the surrounding scope, including visibility, $~, and so on?
No, only locals and dynamic (block) variables. To be honest I always disliked this semantic change too. I think it was a mistake to add it. It will probably be reverted for 1.0. Laurent
On Wed, Jan 26, 2011 at 4:50 PM, Laurent Sansonetti <lsansonetti@apple.com> wrote:
No, only locals and dynamic (block) variables. To be honest I always disliked this semantic change too. I think it was a mistake to add it. It will probably be reverted for 1.0.
I started to try to implement it, and it's not easy. I would need to cascade all the way up the containing scopes, cloning each one. It's very messy. I would have no objections to this behavior going away. What if instead of e.g. async, you had the following form: queue.async(a, b) {|a, b| ... } Because 1.9 forces variables in the block's argument list to be local to the block, this would avoid the sharing of variables completely. This would work with anything proc-like as well: job = ->(a, b) { ... } # block-local a and b queue.async(a, b, &job) # outer scope's a and b Of course you might want to rename them in the block, to avoid confusion, but this neatly does what "tap" did in my suggested fix (and I don't know why I didn't think of it sooner). - Charlie
When I need to get a queue-protected result into a local in code that is concurrent (so I can't use an ivar) is a pointer the best (only) way to get the result of a sync block? Assuming I don't want to factor out a method object. ex: result_p = Pointer.new(:id) some_queue.sync do result_p.assign(value_protected_by_queue) end result = result_p[0] it's not very ruby-ish... Cheerio, Michael Johnston lastobelus@mac.com On 2011-01-26, at 2:50 PM, Laurent Sansonetti wrote:
On Jan 26, 2011, at 12:49 AM, Charles Oliver Nutter wrote:
On Mon, Jan 24, 2011 at 11:13 AM, Joshua Ballanco <jballanc@gmail.com> wrote:
Regarding the potential bug...well...where to begin? So, yes, MacRuby blocks have the same semantics as regular Ruby blocks, *except* when they are dispatched through GCD. In that case, ivars are shared, but local variable get copied. I'm sure Laurent can explain why better than I, but it has to do with the semantics of libdispatch and the uncertainty inherent in when a dispatched block or function will execute (i.e. if local variables were not copied during dispatch, they might go out of scope and be collected before GCD ever gets around to running the code).
Wow, that's very surprising. I'm not sure I agree with bending Ruby semantics so drastically, even to help concurrency. Or at least, I'd expect other threaded scenarios to be consistent:
~ ➔ ruby -ve "a = 0; Thread.new { a += 1 }.join; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 1
~ ➔ ruby -ve "a = 0; q = Dispatch::Queue.concurrent; q.sync {a += 1}; p a" MacRuby 0.8 (ruby 1.9.2) [universal-darwin10.0, x86_64] 0
The implicitness in being able to mutate the surrounding scope is certainly problematic. This is one reason Java's anonymous inner classes require that referenced variables be declared final...to indicate they can't be mutated by the body of the anonymous inner class's methods.
The result is that people end up using one-element arrays and the like, but people find ways around anything.
So I suppose this applies to anything in the surrounding scope, including visibility, $~, and so on?
No, only locals and dynamic (block) variables.
To be honest I always disliked this semantic change too. I think it was a mistake to add it. It will probably be reverted for 1.0.
Laurent _______________________________________________ MacRuby-devel mailing list MacRuby-devel@lists.macosforge.org http://lists.macosforge.org/mailman/listinfo.cgi/macruby-devel
On Saturday, October 22, 2011 at 1:35 AM, Michael Johnston wrote:
When I need to get a queue-protected result into a local in code that is concurrent (so I can't use an ivar) is a pointer the best (only) way to get the result of a sync block? Assuming I don't want to factor out a method object.
ex:
result_p = Pointer.new(:id) some_queue.sync do result_p.assign(value_protected_by_queue) end result = result_p[0]
it's not very ruby-ish...
There's no restriction on not using ivars in block. Indeed, even locals are in scope in a block (and with a #sync dispatched block such as the one you provided, there aren't even any threading issues): result = nil Dispatch::Queue.concurrent.sync do result = true end p result #=> true
Very strange, your example works for me in irb too. Irb must do something odd with locals. Have you tried this outside of irb, though? If you wrap with a method def'n in irb, it behaves differently, and the fact that locals are copied has been discussed lots on the list. But Laurent (I think) said this will likely change, has it already? I'm on 0.10 $ macirb --noprompt def check print "can I change a local scalar? " maybe = "nope" $q.sync do maybe = "yep" end puts maybe end => nil def check_p print "can I assign to a pointer? " maybe_p = Pointer.new(:id) maybe_p.assign "nope" $q.sync do maybe_p.assign "yep" end puts maybe_p[0] end => nil def check_w print "can I assign to a wrapper attr? " maybe = ResultWrapper.new("nope") $q.sync do maybe.value = "yep" end puts maybe.value end => nil ResultWrapper = Struct.new(:value) => ResultWrapper $q= Dispatch::Queue.new('q') => q check can I change a local scalar? nope => nil # but using a pointer works: => nil check_p can I assign to a pointer? yep => nil # or a wrapper (but more expensive for tight loops): => nil check_w can I assign to a wrapper attr? yep => nil $q= Dispatch::Queue.concurrent => com.apple.root.default-priority # double-checking it isn't different for the parallel queues: => nil check can I change a local scalar? nope => nil check_p can I assign to a pointer? yep => nil check_w can I assign to a wrapper attr? yep => nil Cheerio, Michael Johnston lastobelus@mac.com On 2011-10-22, at 12:32 AM, Joshua Ballanco wrote:
On Saturday, October 22, 2011 at 1:35 AM, Michael Johnston wrote:
When I need to get a queue-protected result into a local in code that is concurrent (so I can't use an ivar) is a pointer the best (only) way to get the result of a sync block? Assuming I don't want to factor out a method object.
ex:
result_p = Pointer.new(:id) some_queue.sync do result_p.assign(value_protected_by_queue) end result = result_p[0]
it's not very ruby-ish...
There's no restriction on not using ivars in block. Indeed, even locals are in scope in a block (and with a #sync dispatched block such as the one you provided, there aren't even any threading issues):
result = nil Dispatch::Queue.concurrent.sync do result = true end p result #=> true _______________________________________________ MacRuby-devel mailing list MacRuby-devel@lists.macosforge.org http://lists.macosforge.org/mailman/listinfo.cgi/macruby-devel
Ah, indeed, there are subtle differences with lvars at the top-level in IRB vs in a method scope. Not sure if I'd consider this a bug or not, but to illustrate:
result = nil => nil Dispatch::Queue.concurrent.sync do result = true end => nil result => true def test result = nil Dispatch::Queue.concurrent.sync do result = true end result end => nil test => nil
Anyway, we're getting away from the original question. If you replace your lvars with ivars (s/result/@result/ above), then things will work. Just be careful of the possibility of concurrency bugs when assigning the same ivar from multiple queued blocks (and also have a look at the dispatch gem which has some convenience methods to make this sort of thing easier). On Saturday, October 22, 2011 at 6:30 AM, Michael Johnston wrote:
Very strange, your example works for me in irb too. Irb must do something odd with locals. Have you tried this outside of irb, though? If you wrap with a method def'n in irb, it behaves differently, and the fact that locals are copied has been discussed lots on the list. But Laurent (I think) said this will likely change, has it already? I'm on 0.10
$ macirb --noprompt def check print "can I change a local scalar? " maybe = "nope" $q.sync do maybe = "yep" end puts maybe end => nil def check_p print "can I assign to a pointer? " maybe_p = Pointer.new(:id) maybe_p.assign "nope" $q.sync do maybe_p.assign "yep" end puts maybe_p[0] end => nil def check_w print "can I assign to a wrapper attr? " maybe = ResultWrapper.new("nope") $q.sync do maybe.value = "yep" end puts maybe.value end => nil ResultWrapper = Struct.new(:value) => ResultWrapper $q= Dispatch::Queue.new('q') => q check can I change a local scalar? nope => nil # but using a pointer works: => nil check_p can I assign to a pointer? yep => nil # or a wrapper (but more expensive for tight loops): => nil check_w can I assign to a wrapper attr? yep => nil $q= Dispatch::Queue.concurrent => com.apple.root.default-priority # double-checking it isn't different for the parallel queues: => nil check can I change a local scalar? nope => nil check_p can I assign to a pointer? yep => nil check_w can I assign to a wrapper attr? yep => nil
Cheerio,
Michael Johnston lastobelus@mac.com (mailto:lastobelus@mac.com)
On 2011-10-22, at 12:32 AM, Joshua Ballanco wrote:
On Saturday, October 22, 2011 at 1:35 AM, Michael Johnston wrote:
When I need to get a queue-protected result into a local in code that is concurrent (so I can't use an ivar) is a pointer the best (only) way to get the result of a sync block? Assuming I don't want to factor out a method object.
ex:
result_p = Pointer.new(:id) some_queue.sync do result_p.assign(value_protected_by_queue) end result = result_p[0]
it's not very ruby-ish...
There's no restriction on not using ivars in block. Indeed, even locals are in scope in a block (and with a #sync dispatched block such as the one you provided, there aren't even any threading issues):
result = nil Dispatch::Queue.concurrent.sync do result = true end p result #=> true _______________________________________________ MacRuby-devel mailing list MacRuby-devel@lists.macosforge.org (mailto:MacRuby-devel@lists.macosforge.org) http://lists.macosforge.org/mailman/listinfo.cgi/macruby-devel
_______________________________________________ MacRuby-devel mailing list MacRuby-devel@lists.macosforge.org (mailto:MacRuby-devel@lists.macosforge.org) http://lists.macosforge.org/mailman/listinfo.cgi/macruby-devel
participants (4)
-
Charles Oliver Nutter
-
Joshua Ballanco
-
Laurent Sansonetti
-
Michael Johnston