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.

Sign up for GitHub

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

Jump to bottom

BUG: binding visibility for class_exec not thread safe? #4962

Closed
y-f-u opened this issue Jan 10, 2018 · 6 comments
Closed

BUG: binding visibility for class_exec not thread safe? #4962

y-f-u opened this issue Jan 10, 2018 · 6 comments
Milestone
JRuby 9.2.0.0

Comments

@y-f-u
Copy link

y-f-u commented Jan 10, 2018

Environment

Provide at least:

  • JRuby version (jruby -v) and command line (flags, JRUBY_OPTS, etc)
    • jruby 9.1.15.0 (2.3.3) 2017-12-07 929fde8 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 +indy +jit [darwin-x86_64]
  • Operating system and platform (e.g. uname -a)
    • Darwin MacBook-Pro.local 17.3.0 Darwin Kernel Version 17.3.0: Thu Nov 9 18:09:22 PST 2017; root:xnu-4570.31.3~1/RELEASE_X86_64 x86_64 i386 MacBookPro13,1 Darwin

Expected Behavior

  • Describe your expectation of how JRuby should behave, perhaps by showing how CRuby/MRI behaves.
    • binding visibility for class_exec should be thread_safe
    • class_exec should always use visibility: public
    • should produce same result as running in MRI:
test1
test2
test1
test2
  • Provide an executable Ruby script or a link to an example repository.
$block = nil

def take_a_block(&some_block)
  $block = some_block
end

class ABC
  private

  take_a_block do
    def test1
      puts "test1"
    end

    sleep 1

    def test2
      puts "test2"
    end

    sleep 1
  end
end

ABC

def a_class
  klass = Class.new
  klass.class_exec(&$block)

  klass
end

#succeeded
a_class.new.test1
a_class.new.test2

# failed
Thread.new { a_class.new.test1 }
sleep 1.5
a_class.new.test2

Actual Behavior

  • Describe or show the actual behavior.
    • return a NoMethodError with a message showing calling private method, see below
  • Provide text or screen capture showing the behavior.
test1
test2
test1
NoMethodError: private method `test2' called for #<#<Class:0x1d7acb34>:0x48a242ce>
Did you mean?  test1
@headius
Copy link
Member

headius commented Jan 10, 2018

I believe you are correct, the visibility here is getting stepped on by multiple threads.

The logic for class_exec forces the block's binding to public visibility, restoring it on the way back out again. If another thread is also evaluating the block, it will see the visibility change. That's what you see here when the second method is defined private.

This is a very unusual case, however. Because you're sharing a single global block across all threads, those threads also share state like $~ value.

We can fix this most simply by cloning the block and its binding when we do a class_exec but that will prevent sharing anything, and some of the sharing is intentional.

@y-f-u
Copy link
Author

y-f-u commented Jan 10, 2018 via email

@headius
Copy link
Member

headius commented Jan 10, 2018

It is possible the GIL has some effect, but after looking at CRuby's code a bit I see it does appear to create a new "cref" for the exec. I'll play with our duping and see if I can figure out the equivalent logic.

headius added a commit that referenced this issue Jan 10, 2018
@headius
Clone frame and block before proceeding into class_exec logic.
bd0f3f8 
See #4962
@headius
Copy link
Member

headius commented Jan 10, 2018

I've pushed branch clone_more_block_for_exec with some extra logic to clone the block and the frame associated with it (which is where the visibility is stored). We'll see how it looks in CI.

https://travis-ci.org/jruby/jruby/builds/327409792

@wezm
Copy link

wezm commented May 14, 2018

So it looks like CI was mostly happy. Does this mean the patch can be pushed forward?

@headius
Copy link
Member

headius commented May 14, 2018

@wezm Thanks for pinging! Yes, I think we can merge this in now for 9.2.

@headius headius mentioned this issue May 14, 2018
Merged
@headius headius added this to the JRuby 9.2.0.0 milestone May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
JRuby 9.2.0.0
Development

No branches or pull requests
3 participants
@headius @wezm @y-f-u