rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 358 forks source link

"singleton on non-persistent Java type" warning on JRuby when mocking an instance method on a Java object proxy #1444

Closed ikaronen-relex closed 2 years ago

ikaronen-relex commented 2 years ago

Subject of the issue

As described at https://github.com/jruby/jruby/wiki/Persistence, JRuby issues a warning when accessing the singleton class of a Java object proxy unless the object's class has been explicitly marked as persistent. Such a warning is triggered by the code in RSpec::Mocks::InstanceMethodStasher#initialize when the object argument is a Java object proxy.

Your environment

Steps to reproduce

Run the following spec under JRuby:

describe RSpec::Mocks::InstanceMethodStasher do
  it 'does not warn when mocking a method on a Java proxy object' do
    list = java.util.ArrayList.new
    allow(list).to receive(:foo).and_return(42)
    expect(list.foo).to eq(42)
  end
end

Expected behavior

The spec passes with no warnings printed to standard output or error streams.

Actual behavior

The spec passes, but the following warning is printed to stderr:

<path_to_gems>/rspec-mocks-3.10.0/lib/rspec/mocks/instance_method_stasher.rb:8: warning: singleton on non-persistent Java type Java::JavaUtil::ArrayList (https://github.com/jruby/jruby/wiki/Persistence)
pirj commented 2 years ago

We recommend using other mechanisms to store per-Java-object data

We don't really store just yet, we're getting the singleton class:

@klass = (class << object; self; end)

I recall there's a difference between this and object.singleton_class, like in certain cases one approach is forcing the singleton to be initialized, and the other does not, but it's hard to recall the details.

#singleton_class to my best memory was introduced in Ruby 1.9.x, and we still support 1.8.6 in RSpec 3.

I'd suggest finding discussions here around singleton_class, and see why we decided to stick to this notation in RSpec 4.

Would the use of @klass = obeject.singleton_class fix the warning for you?

ikaronen-relex commented 2 years ago

Would the use of @klass = obeject.singleton_class fix the warning for you?

@pirj No, that gives the same warning. AIUI, simply accessing the singleton class (which causes JRuby to generate one) is enough to trigger the warning.

The problem is that, in order for the singleton class to behave as expected (i.e. the same object always has the same singleton class, and changes to the singleton class don't disappear while the object exists), JRuby needs to set up an internal weak reference cache for wrappers of that particular class, so that any time a given instance of that class is received from a Java method and needs to be given a Ruby wrapper, the same cached wrapper will always be used. That takes time and memory, so JRuby only does that when necessary — and while it can indeed determine when it's likely to be necessary and to auto-enable the cache, it issues a warning if you haven't explicitly indicated that you really want to do that.

So why do I think that RSpec should auto-enable the persistent wrapper cache for mocked Java proxies, then? Well, for one thing, the warning is annoying and seems hard to avoid otherwise, except by littering user code with lots of some.java.pkg.ClassName.__persistent__ = true before mocking any Java proxy object. Also, the warning is printed only once per program execution, so having RSpec trigger it will hide any such warnings from the actual code being tested.

(That said, if you decide that this is something you'd rather not do, I'm OK with that too — I can always either work around it locally with Module#prepend or take the time to add those some.java.pkg.ClassName.__persistent__ = true lines everythere they're needed. But I wanted to at least report this issue so that anyone else searching for it later will find it, even if it ends up being declined.)

ikaronen-relex commented 2 years ago

BTW, here's the prepend work-around I alluded to above:

require 'rspec/mocks'

# work-around for https://github.com/rspec/rspec-mocks/issues/1444
module InstanceMethodStasherFix
  def initialize(object, *rest)
    object.class.__persistent__ = true if object.respond_to?(:java_class)
    super(object, *rest)
  end
end
RSpec::Mocks::InstanceMethodStasher.prepend(InstanceMethodStasherFix)
JonRowe commented 2 years ago

Given this would silently affect a class under test, I'm not sure we want to silence this warning, after all a user can easily fix it themselves by calling the method noticed, I think I'd rather we left this to the end user...

pirj commented 2 years ago

Closing this, as there doesn't seem to be anything actionable on our side. A workaround exists, thanks for coming up with a snippet!