Skip to content

Commit

Permalink
Mutex version of atomic reference.
Browse files Browse the repository at this point in the history
  • Loading branch information
chrisseaton committed May 14, 2014
1 parent 82176a7 commit f187c6f
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 1 deletion.
52 changes: 52 additions & 0 deletions lib/concurrent/atomic/atomic.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
module Concurrent

class MutexAtomic

def initialize(init = nil)
@value = init
@mutex = Mutex.new
end

def value
@mutex.lock
result = @value
@mutex.unlock

result
end

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 15, 2014

Member

it may be nice to have this also aliased as deref.


def value=(value)
@mutex.lock
result = @value = value
@mutex.unlock

result
end

def modify
@mutex.lock
result = yield @value
@value = result
@mutex.unlock

result
end

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 15, 2014

Member

may leave @mutex unlocked. I would choose update instead of modify, what do you thing?

def update
  @mutex.lock
  @value = yield @value
ensure
  @mutex.unlock
end

This comment has been minimized.

Copy link
@chrisseaton

chrisseaton May 15, 2014

Author Member

Yes this is where I suddenly realised it's not safe and opened 79.

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 19, 2014

Member

I've also realized one more thing, this is blocking reading from the reference while the block is running. It may be better not to block reading and rerun the block on collisions, see https://github.com/headius/ruby-atomic/blob/master/lib/atomic/delegated_update.rb.

This comment has been minimized.

Copy link
@chrisseaton

chrisseaton May 19, 2014

Author Member

We could do - but that's a separate operation entirely, which is why Charles called it try_update. I think we could add a try_update, as we have with MVar etc, but we should also have the simpler case of just update. People using try_update have to either run the method in their own loop, which involves boilerplate, or guarantee that they provide a idempotent block, which is easy to get wrong. Both probably aren't ideal for a Ruby developer inexperienced with concurrency.

This comment has been minimized.

Copy link
@jdantonio

jdantonio May 19, 2014

Member

+1 to @chrisseaton. It's paramount that we keep in mind the needs of the average Ruby user who likely has little experience with concurrency. If adding a try_update method will provide a better experience for a developer with greater understanding of concurrency then it may be worth the effort to add it, too.

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 20, 2014

Member

Control question: Are we trying to mimic ruby-atomic API or just create atomic reference? I am assuming yes.

To clarify I meant that our #modify method could be better to mimic #update method from ruby-atomic to ease the transition. (I was not considering try_update) On the other hand is that it may be safer for inexperienced users not to care what they do in the block locking-wise, ruby-atomic behavior can repeat the block on collision which may lead to doubled side-effects but cannot deadlock. Personally I lean towards multiple side-effects (by re-running the block) because that is easier to find.

This comment has been minimized.

Copy link
@mighe

mighe May 20, 2014

Contributor

The real problem is that in both cases we cannot achieve a perfect abstraction, but it will be somehow leaky.
For this class we could implement three different methods

  • lock_update that executes the block inside the mutex (-> to avoid deadlock the block should not acquire any other lock)
  • try_update that returns true if the block has been executed, false otherwise
  • spin_update the lock free version as in ruby-atomic

only this way we will have a real complete API.
I think that AtomicReference is a lower level abstraction, so it should be flexible and avoided by less experienced programmers: we have a lot of simpler component for them

This comment has been minimized.

Copy link
@jdantonio

jdantonio May 20, 2014

Member

I'm going to defer to @mighe on this. My expertise is with the higher level abstractions. @mighe did most of the work to extract and create the lower level abstractions and I've mostly followed his lead in that area.

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 22, 2014

Member

@mighe I like having all three methods.


def compare_and_set(expect, update)
@mutex.lock
if @value == expect

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 15, 2014

Member

eq may be better here.

@value = update
result = true
else
result = false
end
@mutex.unlock

result
end
end

This comment has been minimized.

Copy link
@pitr-ch

pitr-ch May 15, 2014

Member

I like concise methods but this is possibly too clever at the expense of readability.

def compare_and_set(expect, update)
  @mutex.lock
  @value == expect and (@value = update; true)
ensure
  @mutex.unlock
end
# or
def compare_and_set(expect, update)
  @mutex.lock
  @value == expect and begin
    @value = update
    true
  end
ensure
  @mutex.unlock
end

class Atomic < MutexAtomic
end

end
1 change: 0 additions & 1 deletion lib/concurrent/atomic/atomic_fixnum.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ def decrement
#
# @return [Boolean] true if the value was updated else false
def compare_and_set(expect, update)

@mutex.lock
if @value == expect
@value = update
Expand Down
1 change: 1 addition & 0 deletions lib/concurrent/atomics.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'concurrent/atomic/atomic'
require 'concurrent/atomic/atomic_boolean'
require 'concurrent/atomic/atomic_fixnum'
require 'concurrent/atomic/condition'
Expand Down
133 changes: 133 additions & 0 deletions spec/concurrent/atomic/atomic_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
require 'spec_helper'

share_examples_for :atomic do

context 'construction' do

it 'sets the initial value' do
described_class.new(:foo).value.should eq :foo
end

it 'defaults the initial value to nil' do
described_class.new.value.should eq nil
end
end

context '#value' do

it 'returns the current value' do
counter = described_class.new(:foo)
counter.value.should eq :foo
end
end

context '#value=' do

it 'sets the #value to the given object' do
atomic = described_class.new(:foo)
atomic.value = :bar
atomic.value.should eq :bar
end

it 'returns the new value' do
atomic = described_class.new(:foo)
(atomic.value = :bar).should eq :bar
end
end

context '#modify' do

it 'yields the current value' do
atomic = described_class.new(:foo)
current = []
atomic.modify { |value| current << value }
current.should eq [:foo]
end

it 'stores the value returned from the yield' do
atomic = described_class.new(:foo)
atomic.modify { |value| :bar }
atomic.value.should eq :bar
end

it 'returns the new value' do
atomic = described_class.new(:foo)
atomic.modify{ |value| :bar }.should eq :bar
end
end

context '#compare_and_set' do

it 'returns false if the value is not found' do
described_class.new(:foo).compare_and_set(:bar, :foo).should eq false
end

it 'returns true if the value is found' do
described_class.new(:foo).compare_and_set(:foo, :bar).should eq true
end

it 'sets if the value is found' do
f = described_class.new(:foo)
f.compare_and_set(:foo, :bar)
f.value.should eq :bar
end

it 'does not set if the value is not found' do
f = described_class.new(:foo)
f.compare_and_set(:bar, :baz)
f.value.should eq :foo
end
end
end

module Concurrent

describe MutexAtomic do

it_should_behave_like :atomic

specify 'construction is synchronized' do
mutex = double('mutex')
Mutex.should_receive(:new).once.with(no_args).and_return(mutex)
described_class.new
end

specify 'value is synchronized' do
mutex = double('mutex')
Mutex.stub(:new).with(no_args).and_return(mutex)
mutex.should_receive(:lock)
mutex.should_receive(:unlock)
described_class.new.value
end

specify 'value= is synchronized' do
mutex = double('mutex')
Mutex.stub(:new).with(no_args).and_return(mutex)
mutex.should_receive(:lock)
mutex.should_receive(:unlock)
described_class.new.value = 10
end

specify 'modify is synchronized' do
mutex = double('mutex')
Mutex.stub(:new).with(no_args).and_return(mutex)
mutex.should_receive(:lock)
mutex.should_receive(:unlock)
described_class.new(:foo).modify { |value| value }
end

specify 'compare_and_set is synchronized' do
mutex = double('mutex')
Mutex.stub(:new).with(no_args).and_return(mutex)
mutex.should_receive(:lock)
mutex.should_receive(:unlock)
described_class.new(14).compare_and_set(14, 2)
end
end

describe Atomic do
it 'inherits from MutexAtomic' do
Atomic.ancestors.should include(MutexAtomic)
end
end
end

0 comments on commit f187c6f

Please sign in to comment.