ConstructorInvoker isn't storing the proxy in the ObjectProxyCache #3576

Closed
smellsblue opened this Issue Dec 31, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@smellsblue
Contributor

smellsblue commented Dec 31, 2015

We are defining singleton methods on Java objects that had been freshly constructed. These objects get stored on the Java side, and then later retrieved again from the JRuby side, only to have their singleton methods missing. We are holding on to the original proxy from the constructed objects, so this shouldn't be a case of a garbage collected proxy object.

I started tracing JRuby with my scenario and found the code path I was hitting in ConstructorInvoker wasn't actually caching the JavaProxy in the ObjectProxyCache, while if I had gone through an InstanceMethodInvoker path, it would have been cached.

Below is a simple Minitest test case that fails, demonstrating the problem.

// Test.java
public class Test {
    private static Test lastInstance;

    public Test() {
        lastInstance = this;
    }

    public static Test getLastInstance() {
        return lastInstance;
    }
}
# test.rb
gem "minitest"
require "minitest/autorun"
Java::Default::Test.__persistent__ = true

class TestCaching < Minitest::Test
  def test_constructed_proxy_is_cached
    t = Java::Default::Test.new
    def t.testing; 42; end
    assert_equal 42, t.testing
    assert_equal t, Java::Default::Test.last_instance
    assert_equal 42, Java::Default::Test.last_instance.testing
  end
end

The results of running on 1.7.22 and 9.0.4.0:

$ rvm jruby-1.7.22 do ruby test.rb
Run options: --seed 12875

# Running:

E

Finished in 0.006000s, 166.6667 runs/s, 333.3333 assertions/s.

  1) Error:
TestCaching#test_constructed_proxy_is_cached:
NoMethodError: undefined method `testing' for #<Java::Default::Test:0x53fdffa1>
    test.rb:11:in `test_constructed_proxy_is_cached'

1 runs, 2 assertions, 0 failures, 1 errors, 0 skips
$ rvm jruby-9.0.4.0 do ruby test.rb
Run options: --seed 9484

# Running:

E

Finished in 0.009849s, 101.5313 runs/s, 203.0626 assertions/s.

  1) Error:
TestCaching#test_constructed_proxy_is_cached:
NoMethodError: undefined method `testing' for #<Java::Default::Test:0x1a78dacd>
    test.rb:11:in `test_constructed_proxy_is_cached'

1 runs, 2 assertions, 0 failures, 1 errors, 0 skips

Some additional notes:

  • In our codebase this works on 1.7.22, but this test fails in both (in our case, more is happening, so maybe that extra stuff is triggering a working path in 1.7.22 somehow)
  • If you drop Java::Default::Test.__persistent__ = true from the test, it starts passing in 9.0.4.0, but then if you create another test method that does the same thing, the second test fails (presumably the code triggering the warning about persistence is somehow getting the proxy object to be cached?)
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 31, 2015

Member

I think your theory is right...the ConstructorInvoker must just be ignoring the persistent flag. Do you want to attempt to patch it?

Member

headius commented Dec 31, 2015

I think your theory is right...the ConstructorInvoker must just be ignoring the persistent flag. Do you want to attempt to patch it?

@smellsblue

This comment has been minimized.

Show comment
Hide comment
@smellsblue

smellsblue Dec 31, 2015

Contributor

@headius absolutely! Though I probably won't have time until next week; is that ok?

Contributor

smellsblue commented Dec 31, 2015

@headius absolutely! Though I probably won't have time until next week; is that ok?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 1, 2016

Member

Hey, it's your bug :-) We won't be getting a release out in the next week
anyway, so there's no rush.

@headius https://github.com/headius absolutely! Though I probably won't
have time until next week; is that ok?


Reply to this email directly or view it on GitHub
#3576 (comment).

Member

headius commented Jan 1, 2016

Hey, it's your bug :-) We won't be getting a release out in the next week
anyway, so there's no rush.

@headius https://github.com/headius absolutely! Though I probably won't
have time until next week; is that ok?


Reply to this email directly or view it on GitHub
#3576 (comment).

@bitfurry

This comment has been minimized.

Show comment
Hide comment
@bitfurry

bitfurry Jan 3, 2016

Contributor

@headius @smellsblue I tried to debug this, got that CustructorInvoker is not getting any block, due to which setCacheProxy is not getting called.
Any further directions would be very helpful.

Contributor

bitfurry commented Jan 3, 2016

@headius @smellsblue I tried to debug this, got that CustructorInvoker is not getting any block, due to which setCacheProxy is not getting called.
Any further directions would be very helpful.

smellsblue added a commit to smellsblue/jruby that referenced this issue Jan 4, 2016

@kares kares closed this in #3581 Jan 11, 2016

@kares kares added this to the JRuby 9.0.5.0 milestone Jan 11, 2016

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