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

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

Closed
fevin86 opened this Issue Jan 10, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@fevin86

fevin86 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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 10, 2018

Member

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.

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.

@fevin86

This comment has been minimized.

Show comment
Hide comment
@fevin86

fevin86 Jan 10, 2018

fevin86 commented Jan 10, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 10, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 10, 2018

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@wezm

wezm May 14, 2018

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

wezm commented May 14, 2018

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 14, 2018

Member

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

Member

headius commented May 14, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment