Skip to content
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

Make CRuby Set thread safe #847

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions lib/concurrent-ruby/concurrent/set.rb
Expand Up @@ -23,9 +23,21 @@ module Concurrent
# @!macro internal_implementation_note
SetImplementation = case
when Concurrent.on_cruby?
# Because MRI never runs code in parallel, the existing
# non-thread-safe structures should usually work fine.
::Set
# The CRuby implementation of Set is written in Ruby itself and is
# not thread safe for certain methods.
require 'monitor'
require 'concurrent/thread_safe/util/data_structures'

class CRubySet < ::Set
def initialize(*args)
self.send(:_mon_initialize)
Copy link
Contributor Author

@fzakaria fzakaria Feb 10, 2020

Choose a reason for hiding this comment

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

Without this -- certain Sets are being created not through initialize
I would see:

when initializing with a block argument
concurrent-ruby/concurrent/thread_safe/util/data_structures.rb:43: 
warning: instance variable @_monitor not initialized

I'm not certain how the Set is being initialized without going through:

 def self.new(*args)
    obj = super(*args)
    obj.send(:_mon_initialize)
    obj
  end

TBH i'm

super(*args)
end
end

# make use of the same RBX
ThreadSafe::Util.make_synchronized_on_rbx Concurrent::CRubySet
CRubySet

when Concurrent.on_jruby?
require 'jruby/synchronized'
Expand Down
Expand Up @@ -17,7 +17,7 @@ def self.make_synchronized_on_rbx(klass)
private

def _mon_initialize
@_monitor = Monitor.new unless @_monitor # avoid double initialisation
@_monitor ||= Monitor.new # avoid double initialisation
end

def self.new(*args)
Expand Down
29 changes: 28 additions & 1 deletion spec/concurrent/set_spec.rb
Expand Up @@ -42,7 +42,7 @@ module Concurrent
end

context 'concurrency' do
it do
it 'simple' do
(1..Concurrent::ThreadSafe::Test::THREADS).map do |i|
in_thread do
1000.times do
Expand All @@ -55,6 +55,33 @@ module Concurrent
end.map(&:join)
expect(set).to be_empty
end

it 'force context switch' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

writing a test for this is actually not easy lol.

What this ended up testing was that during a method of set it grabs a monitor and locks out any other changes.

expect(set.add?(1)).to eq set

the above line should occasionally return nil if the methods were not synchronized.

barrier = Concurrent::CyclicBarrier.new(2)

# methods like include? or delete? are implemented for CRuby in Ruby itself
# @see https://github.com/ruby/ruby/blob/master/lib/set.rb
set.clear

# add a single element
set.add(1)

# This thread should start and `Set#reject!` in CRuby should cache a value of `0` for size
thread_reject = in_thread do
# we expect this to return nil since nothing should have changed.
expect(set.reject! do |v|
barrier.wait
v == 1 # only delete the 1 value
end).to eq set
end

thread_add = in_thread do
barrier.wait
expect(set.add?(1)).to eq set
end

join_with [thread_reject, thread_add]
end
end
end
end