jmock-developers / jmock-library

An expressive Mock Object library for Test Driven Development
http://www.jmock.org
BSD 3-Clause "New" or "Revised" License
133 stars 70 forks source link

Mocks being finalized report "the Mockery is not thread-safe: use a Synchroniser to ensure thread safety" (JMOCK-256) #36

Closed sf105 closed 11 years ago

sf105 commented 11 years ago

from John Stoneham http://jira.codehaus.org/browse/JMOCK-256

In using JMock to test Grails classes, I'm using the 'grails interactive' mode which can repeatedly invoke test cases without shutting down the JVM. My tests all extend a common superclass that uses reflection to introspect and clear out instance variables, including my Mockery instances, so all of these instances are available for garbage collection. We're using ClassImposteriser as well.

The finalizer comes along and runs finalize() on the mocks. That invocation comes along into the proxy's callback within ClassImposteriser which gets into SingleThreadedPolicy's checkRunningOnTestThread method, but you're on the finalizer thread, not the test thread, so you get the thread-safety message.

sf105 commented 11 years ago

DuncanMcGregor:

A simple workaround, especially for those of us who often run multi-threaded tests anyway, is just to install a Synchroniser and be done with it

public class JUnitRuleClassImposterisingMockery extends JUnitRuleMockery {

    private final Synchroniser synchroniser = new Synchroniser();

    { 
        setImposteriser(ClassImposteriser.INSTANCE); 
        setThreadingPolicy(synchroniser); // otherwise we get errors on the finalizer thread 
    }
    public Synchroniser getSynchroniser() { 
            return synchroniser; 
    }
}

I can't help thinking that this should be a reasonably high priority, as it affects every use of ClassImposterizer.

dmcg commented 11 years ago

I wonder why this only shows up with the ClassImposterizer?

npryce commented 11 years ago

Because you need to mock a class to get an instance with a finaliser that calls mocked methods

dmcg commented 11 years ago

Both ClassImposteriser and JavaReflectionImposteriser end up creating an instance of Object (or a subclass) that delegates to mockObject.invoke(...) Instances of Object have a finalizer. I'm not seeing why the Proxy.newProxyInstance one is not having it's finalizer invoked.

npryce commented 11 years ago

A java.lang.reflect.Proxy probably has a final implementation of finalize() that doesn't forward the method on to the Mockery. A mocked class will not, or, perhaps, it has a final implementation that invokes mock objects in its fields.

dmcg commented 11 years ago

Interesting - finalize seems to be special

    private Method lastInvokedMethod = null;

    @Test
    public void finalize_methods_seem_to_disappear_on_proxies() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {
        final Method lengthMethod = CharSequence.class.getDeclaredMethod("length");
        final Method finalizeMethod = Object.class.getDeclaredMethod("finalize");
        final Method equalsMethod = Object.class.getDeclaredMethod("equals", new Class[] {Object.class});

        InvocationHandler handler = new InvocationHandler() {
            @Override
            public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
                lastInvokedMethod = method;
                if (method.equals(lengthMethod))
                    return 42;
                else if (method.equals(equalsMethod))
                    return true;
                else
                    return null;
            }
        };
        CharSequence proxy = (CharSequence) Proxy.newProxyInstance(this.getClass().getClassLoader(), new Class<?>[]{CharSequence.class}, handler);

        // check that invocationHandler is working via reflection
        lastInvokedMethod = null;
        assertEquals(42, invokeMethod(proxy, lengthMethod));
        assertEquals(lengthMethod, lastInvokedMethod);

        // check that other methods defined on Object are delegated
        lastInvokedMethod = null;
        assertEquals(true, invokeMethod(proxy, equalsMethod, "banana"));
        assertEquals(equalsMethod, lastInvokedMethod);

        // now show that a call to finalize is not delegated
        lastInvokedMethod = null;
        invokeMethod(proxy, finalizeMethod);
        assertNull(lastInvokedMethod);
    }

    private Object invokeMethod(Object object, Method method, Object... args) throws IllegalAccessException, InvocationTargetException {
        method.setAccessible(true);
        return method.invoke(object, args);
    }
dmcg commented 11 years ago

So what to do? Should we add a filter to the cglib to discard finalize, making it compatible with the Proxy version at the expense of being able to test finalization behaviour?

npryce commented 11 years ago

JMock already treats finalization as special. You can't set expectations on finalize() and it's faked (with a no-op) by the ProxiedObjectIdentity decorator that the mockery wraps around imposters.

--Nat

On 6 January 2013 16:40, dmcg notifications@github.com wrote:

So what to do? Should we add a filter to the cglib to discard finalize, making it compatible with the Proxy version at the expense of being able to test finalization behaviour?

— Reply to this email directly or view it on GitHubhttps://github.com/jmock-developers/jmock-library/issues/36#issuecomment-11930652.

dmcg commented 11 years ago

OK, so we now filter finalizers in CGLIB

npryce commented 11 years ago

What if a mocked class has a final finalizer that invokes a mock object through a field?

On Sunday, January 6, 2013, dmcg wrote:

OK, so we now filter finalizers in CGLIB

— Reply to this email directly or view it on GitHubhttps://github.com/jmock-developers/jmock-library/issues/36#issuecomment-11931497.

dmcg commented 11 years ago

I think that the behaviour for cglib and java.lang.reflect.Proxies is now the same. Can you provide a test case ;-?

tobyweston commented 11 years ago

Sorry. Just to be clear then, are the workarounds from the original ticket no longer needed with 2.6.0? I still get the warnings without it.

The workaround was along these lines (copying over as the original ticket is no longer available :unamused:)

public static class SingleThreadedPolicyAvoidingFinaliseProblems extends SingleThreadedPolicy {
    @Override
    public Invokable synchroniseAccessTo(final Invokable mockObject) {
        final Invokable synchronizedMockObject = super.synchroniseAccessTo(mockObject);
        return new Invokable() {
            @Override
            public Object invoke(Invocation invocation) throws Throwable {
                if (Thread.currentThread().getName().equalsIgnoreCase("Finalizer"))
                    return mockObject.invoke(invocation);
                return synchronizedMockObject.invoke(invocation);
            }
        };
    }
}
    private final Mockery context = new JUnit4Mockery() {{
        setThreadingPolicy(new SingleThreadedPolicyAvoidingFinaliseProblems());
    }};
dmcg commented 11 years ago

Curious. I've committed MockeryFinalizationAcceptanceTests, but while they fail before the fix, and pass after it, they do still fail intermittently when run in ant. So maybe this is less fixed than I thought - I'll reopen.

npryce commented 11 years ago

Here's a ThreadingPolicy we wrote to track down these issues. The functionality could be added to the default jMock policy.

https://gist.github.com/npryce/4706923

dmcg commented 11 years ago

OK, so the last great singleton is the finalization queue. The test is failing because of something else run before it...

dmcg commented 11 years ago

ErrorMessagesAcceptanceTests.testCannotExpectFinalize turns out to be the culprit, and shows that we still do output the message for the case of a type with a public finalizer method. I'm going to close this as fixed and raise another issue for that special case. FWIW the fix should have been in 2.6.0 - @tobyweston could you give a test case to reproduce the issue that you saw?

puntogil commented 11 years ago

hi with jmock 2.6.0 java version "1.7.0_40" OpenJDK Runtime Environment (fedora-2.4.2.1.fc19-i386 u40-b60) OpenJDK Server VM (build 24.0-b56, mixed mode)

Exception in thread "Interrupter-Thread-7" java.util.ConcurrentModificationException: the Mockery is not thread-safe: use a Synchroniser to ensure thread safety the Mockery is not thread-safe: use a Synchroniser to ensure thread safety at org.jmock.internal.SingleThreadedPolicy.reportError(SingleThreadedPolicy.java:35) at org.jmock.internal.SingleThreadedPolicy.checkRunningOnTestThread(SingleThreadedPolicy.java:28) at org.jmock.internal.SingleThreadedPolicy.access$000(SingleThreadedPolicy.java:10) at org.jmock.internal.SingleThreadedPolicy$1.invoke(SingleThreadedPolicy.java:20) at org.jmock.lib.legacy.ClassImposteriser$4.invoke(ClassImposteriser.java:136) at com.google.code.tempusfugit.temporal.Clock$$EnhancerByCGLIB$$d682c32f.create() at com.google.code.tempusfugit.temporal.StopWatch.markAndGetTotalElapsedTime(StopWatch.java:44) at com.google.code.tempusfugit.temporal.Timeout.hasExpired(Timeout.java:48) at com.google.code.tempusfugit.temporal.WaitFor.waitUntil(WaitFor.java:54) at com.google.code.tempusfugit.concurrency.Interrupter$1.run(Interrupter.java:59) at java.lang.Thread.run(Thread.java:724) Exception in thread "Interrupter-Thread-8" java.util.ConcurrentModificationException: the Mockery is not thread-safe: use a Synchroniser to ensure thread safety at org.jmock.internal.SingleThreadedPolicy.reportError(SingleThreadedPolicy.java:35) at org.jmock.internal.SingleThreadedPolicy.checkRunningOnTestThread(SingleThreadedPolicy.java:28) at org.jmock.internal.SingleThreadedPolicy.access$000(SingleThreadedPolicy.java:10) at org.jmock.internal.SingleThreadedPolicy$1.invoke(SingleThreadedPolicy.java:20) at org.jmock.lib.legacy.ClassImposteriser$4.invoke(ClassImposteriser.java:136) at com.google.code.tempusfugit.temporal.Clock$$EnhancerByCGLIB$$d682c32f.create() at com.google.code.tempusfugit.temporal.StopWatch.markAndGetTotalElapsedTime(StopWatch.java:44) at com.google.code.tempusfugit.temporal.Timeout.hasExpired(Timeout.java:48) at com.google.code.tempusfugit.temporal.WaitFor.waitUntil(WaitFor.java:54) at com.google.code.tempusfugit.concurrency.Interrupter$1.run(Interrupter.java:59) at java.lang.Thread.run(Thread.java:724)