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

Concurrent::Set is not thread-safe in CRuby #796

Closed
fxn opened this issue Feb 3, 2019 · 8 comments · Fixed by #911
Closed

Concurrent::Set is not thread-safe in CRuby #796

fxn opened this issue Feb 3, 2019 · 8 comments · Fixed by #911
Assignees
Labels
bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue.

Comments

@fxn
Copy link
Contributor

fxn commented Feb 3, 2019

Concurrent::Set returns Set in CRuby with this argument:

# Because MRI never runs code in parallel, the existing
# non-thread-safe structures should usually work fine.
::Set

As discussed in #793, that is not enough for thread-safety, you also need the interface to be atomic somehow. For Hash and Array this holds because they are written in C, but Set is partially written in Ruby.

To demonstrate Concurrent::Set is not thread-safe I have created this artificial example:

require "concurrent-ruby"

class MySet < Concurrent::Set
  # Note include? is written in Ruby in Set, like delete?
  def include?(o)
    puts "include? call issued, 1 belongs to self, but there is a context switch"
    sleep 2
    super
  end
end

set = MySet[1]

t = Thread.new do
  sleep 1
  puts "clearing the set in a different thread"
  set.clear
end

p set.delete?(1) # should be self, but it is nil

t.join

By the time delete? is invoked, the set has the element 1, therefore, that method should return self (the set). However, it returns nil because clear was called during a context switch in the middle of the method execution.

@fxn
Copy link
Contributor Author

fxn commented Feb 4, 2019

I have revised the example to have the context switch happen in the middle of the execution of delete?.

@thedarkone
Copy link
Contributor

Hi @fxn,

you are right, the comment should be dropped (it is incorrect).

# Because MRI never runs code in parallel, the existing
# non-thread-safe structures should usually work fine.


Furthermore, currently it looks like Set is not safe to use beyond basic add/remove/include? methods. I knew Set was written in Ruby, and I skimmed through the methods while reviewing #666, it looked fine to me 😞 .

@pitr-ch pitr-ch added bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue. labels Feb 7, 2019
@pitr-ch
Copy link
Member

pitr-ch commented Feb 7, 2019

@fxn thanks for filing the issue!
@haridutt12 would you be interested in taking a stab at fixing it?

@anuragshopify
Copy link

I'd like to work on this. What would the fix entail though? Changing the implementation in mri ruby or providing a thread safe interface here?

@pitr-ch
Copy link
Member

pitr-ch commented Aug 24, 2019

@anuragshopify thank you! I've assigned the issue to you. I would recommend to inherit from the MRI's Set in concurrent-ruby and fixing the methods in the new class here. Then you could try to contribute it back to MRI.

@pitr-ch pitr-ch added this to Nope in Hackathon Aug 24, 2019
@pitr-ch pitr-ch assigned fzakaria and unassigned anuragshopify Feb 10, 2020
fzakaria added a commit to fzakaria/concurrent-ruby that referenced this issue Feb 10, 2020
Follow similar pattern for Rbx to wrap every superclass (::Set) instance
method with a Monitor (which is re-entrant).

Included is a test that I believe *would* occasionally fail
if Set was not thread safe.

fixes ruby-concurrency#796
@fzakaria
Copy link
Contributor

@pitr-ch
Copy link
Member

pitr-ch commented Feb 16, 2020

@fxn thank you for the PR! I have a proper look in few days. I've run out of time today.

ruby-concurrency/concurrent-ruby automation moved this from Backlog to Done Jun 4, 2021
@fxn
Copy link
Contributor Author

fxn commented Jun 4, 2021

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue.
5 participants