Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

AtomicReference: update try_update API (resolves #330) #336

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 19, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
### Next Release v0.9.0 (Target Date: 7 June 2015)


* Updated `AtomicReference`
- `AtomicReference#try_update` now simply returns instead of raising exception
- `AtomicReference#try_update!` was added to raise exceptions if an update
fails. Note: this is the same behavior as the old `try_update`
* Pure Java implementations of
- `AtomicBoolean`
- `AtomicFixnum`
Expand Down Expand Up @@ -58,7 +63,7 @@
- `Channel`
- `Exchanger`
- `LazyRegister`
- **new Future Framework** <http://ruby-concurrency.github.io/concurrent-ruby/Concurrent/Edge.html> - unified
- **new Future Framework** <http://ruby-concurrency.github.io/concurrent-ruby/Concurrent/Edge.html> - unified
implementation of Futures and Promises which combines Features of previous `Future`,
`Promise`, `IVar`, `Event`, `Probe`, `dataflow`, `Delay`, `TimerTask` into single framework. It uses extensively
new synchronization layer to make all the paths **lock-free** with exception of blocking threads on `#wait`.
Expand All @@ -80,7 +85,7 @@
- Add AbstractContext#default_executor to be able to override executor class wide
- Add basic IO example
- Documentation somewhat improved
- All messages should have same priority. It's now possible to send `actor << job1 << job2 << :terminate!` and
- All messages should have same priority. It's now possible to send `actor << job1 << job2 << :terminate!` and
be sure that both jobs are processed first.
* Refactored `Channel` to use newer synchronization objects
* Added `#reset` and `#cancel` methods to `TimerSet`
Expand Down Expand Up @@ -162,7 +167,7 @@ Please see the [roadmap](https://github.com/ruby-concurrency/concurrent-ruby/iss
- `SerializedExecutionDelegator` for serializing *any* executor
* Updated `Async` with serialized execution
* Updated `ImmediateExecutor` and `PerThreadExecutor` with full executor service lifecycle
* Added a `Delay` to root `Actress` initialization
* Added a `Delay` to root `Actress` initialization
* Minor bug fixes to thread pools
* Refactored many intermittently failing specs
* Removed Java interop warning `executor.rb:148 warning: ambiguous Java methods found, using submit(java.lang.Runnable)`
Expand Down
34 changes: 31 additions & 3 deletions lib/concurrent/atomic_reference/direct_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module AtomicDirectUpdate
# Pass the current value to the given block, replacing it
# with the block's result. May retry if the value changes
# during the block's execution.
#
#
# @yield [Object] Calculate a new value for the atomic reference using
# given (old) value
# @yieldparam [Object] old_value the starting value of the atomic reference
Expand All @@ -27,17 +27,45 @@ def update
# @!macro [attach] atomic_reference_method_try_update
#
# Pass the current value to the given block, replacing it
# with the block's result. Return nil if the update fails.
#
# @yield [Object] Calculate a new value for the atomic reference using
# given (old) value
# @yieldparam [Object] old_value the starting value of the atomic reference
#
# @note This method was altered to avoid raising an exception by default.
# Instead, this method now returns `nil` in case of failure. For more info,
# please see: https://github.com/ruby-concurrency/concurrent-ruby/pull/336
#
# @return [Object] the new value, or nil if update failed
def try_update
old_value = get
new_value = yield old_value

return unless compare_and_set old_value, new_value

new_value
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider returning [success, new_value] because nil cannot be distinguished from new value nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be somewhat consistent with the similar function in AtomicMarkableReference. In that class we return an immutable array with [new_val, new_mark]. We could have both methods return a list with true/false as the first element. Having worked in Erlang for a few years I'm very fond of this pattern. It's very common in Erlang to return a tuple of {ok} or {error message} from functions.

If we want to go in this direction we should consider using either ImmutableStruct or Maybe. I added the latter specifically for situations like this. Maybe (aka Option) is a common pattern in functional languages and libraries that I'm very fond of but that I haven't seen much in Ruby.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that returning [success, new_value] might start adding unneccesary verbosity to the API. When you think about it, AtomicMarkableReference would have to then return, [success, [new_mark, new_value]] which seems needlessly complex IMO. Java doesn't even add that level of verbosity to the API 😆

Another alternative might be to initialize the value with some sort of private EmptyObject or something which is used as a placeholdern in order to distinguish from nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, the problem is that idiomatic Ruby abuses nil in ways that can make return values ambiguous. That's why I created the obligation API (#value and #reason) methods very early on. It removes the ambiguity. I don't have a strong opinion but I'm comfortable leaving it returning nil. For a couple of reasons. Mostly because that's the way the API previously worked. I believe that the baseline assumption is that we do not change APIs unless there is a compelling reason to do so. Secondly, returning nil is idiomatic Ruby, whether we like it or not. Finally, since the user is passing a block to the function it should be clear when looking at the code if nil is a valid return value, and the block can be refactored if necessary.

I'm going to merge this PR now and we can revisit later if we think there may be a better way to handle this return value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 We could add a note to the documentation though to warn the users. If they need to distinguish nil, compare_and_set can be always used. From my side we've considered [success, new_value] and it's a bad idea.


# @!macro [attach] atomic_reference_method_try_update!
#
# Pass the current value to the given block, replacing it
# with the block's result. Raise an exception if the update
# fails.
#
#
# @yield [Object] Calculate a new value for the atomic reference using
# given (old) value
# @yieldparam [Object] old_value the starting value of the atomic reference
#
# @note This behavior mimics the behavior of the original
# `AtomicReference#try_update` API. The reason this was changed was to
# avoid raising exceptions (which are inherently slow) by default. For more
# info: https://github.com/ruby-concurrency/concurrent-ruby/pull/336
#
# @return [Object] the new value
#
# @raise [Concurrent::ConcurrentUpdateError] if the update fails
def try_update
def try_update!
old_value = get
new_value = yield old_value
unless compare_and_set(old_value, new_value)
Expand Down
22 changes: 20 additions & 2 deletions spec/concurrent/atomic/atomic_reference_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@
expect(res).to eq 1001
end

specify :test_try_update_bang do
# use a number outside JRuby's fixnum cache range, to ensure identity is preserved
atomic = described_class.new(1000)
res = atomic.try_update! {|v| v + 1}

expect(atomic.value).to eq 1001
expect(res).to eq 1001
end

specify :test_swap do
atomic = described_class.new(1000)
res = atomic.swap(1001)
Expand All @@ -42,12 +51,21 @@
end

specify :test_try_update_fails do
# use a number outside JRuby's fixnum cache range, to ensure identity is preserved
atomic = described_class.new(1000)
expect(
# assigning within block exploits implementation detail for test
atomic.try_update {|v| atomic.value = 1001 ; v + 1}
).to be_falsey
end

specify :test_try_update_bang_fails do
# use a number outside JRuby's fixnum cache range, to ensure identity is preserved
atomic = described_class.new(1000)
expect {
# assigning within block exploits implementation detail for test
atomic.try_update{|v| atomic.value = 1001 ; v + 1}
}.to raise_error(Concurrent::ConcurrentUpdateError)
atomic.try_update! {|v| atomic.value = 1001 ; v + 1}
}.to raise_error Concurrent::ConcurrentUpdateError
end

specify :test_update_retries do
Expand Down