MethodInvocationHelper not thread safe?

[Baron]

One of our tests uses a data provider to execute over 16000 test
cases. When we run the test single threaded it always succeeds.
However, when we run it with multiple data provider threads, it fails
intermittently but always with the same stack trace:

java.lang.IllegalAccessException: Class
org.testng.internal.MethodInvocationHelper can not access a member of
class
org.springframework.test.context.testng.AbstractTestNGSpringContextTests
with modifiers "protected"
at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:65)
at java.lang.reflect.Method.invoke(Method.java:588)
at
org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:
74)
at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:
525)
at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:202)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:613)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:846)
at
org.testng.internal.TestMethodWithDataProviderMethodWorker.call(TestMethodWithDataProviderMethodWorker.java:
73)
at
org.testng.internal.TestMethodWithDataProviderMethodWorker.call(TestMethodWithDataProviderMethodWorker.java:
14)
at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
at java.util.concurrent.FutureTask.run(FutureTask.java:138)
at java.util.concurrent.ThreadPoolExecutor
$Worker.runTask(ThreadPoolExecutor.java:886)
at java.util.concurrent.ThreadPoolExecutor
$Worker.run(ThreadPoolExecutor.java:908)
at java.lang.Thread.run(Thread.java:619)

Looking at MethodInvocationHelper.invoke method, I notice this code:

try {
if (!isPublic) {
thisMethod.setAccessible(true);
}
result = thisMethod.invoke(instance, parameters);
} finally {
if (!isPublic) {
thisMethod.setAccessible(false);
}
}

If multiple threads call this code for the same target method, it
appears to be a race condition as to whether the method accessibility
with be true or false at the time invoke is called. That would explain
the "protected" complaint.

I am going to explore adding a "synchronized" around the entire "try/
finally" but while investigating this, I wanted to see what others
more familiar with the code think. Also, has anyone else experienced
this problem with their tests?

Thanks,
Baron

[Cédric Beust ♔]

Hi Baron,

Synchronizing this block looks like the right solution, can you try on your code base and report back? I'll be happy to integrate this patch to TestNG if it works for you.

Thanks.

-- 

Cédric

--
You received this message because you are subscribed to the Google Groups "testng-users" group.
To post to this group, send email to testng...@googlegroups.com.
To unsubscribe from this group, send email to testng-users...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/testng-users?hl=en.

[Baron]

Thanks Cédric for the quick reply!

I will try this out but I am a bit concerned or at least confused. If
I synchronize around thisMethod, I am effectively single threading all
calls to that method, which seems heavy handed when what I want to do
is just manage the accessibility. The confusion I have is why we would
be invoking a non-public method from the test? While the help might
allow us to do that, why would that be occurring within my test such
that TestNG would be invoking a non-public method?

Baron

On Jun 9, 11:37 pm, Cédric Beust ♔ <ced...@beust.com> wrote:
> Hi Baron,
>
> Synchronizing this block looks like the right solution, can you try on your
> code base and report back? I'll be happy to integrate this patch to TestNG
> if it works for you.
>
> Thanks.
>
> --
> Cédric
>

[Baron]

Would it make more sense to just set the accessibility to true and
leave it?

[Cédric Beust ♔]

Hi Baron,

On Fri, Jun 10, 2011 at 11:20 AM, Baron <baron....@gmail.com> wrote:

Would it make more sense to just set the accessibility to true and
leave it?

Indeed. I was trying to be clean with my implementation but I don't think there is much risk in leaving the accessibility to true.

Can you verify that this fixes your problem? When you confirm, I'll make the change and I'll check it in.

-- 

Cédric

[Baron]

Will do. I'm away from my keyboard this week but will report back
early next week.

Baron

[Baron]

I have implemented and verified the fix for the method accessibility
race condition. The fix was to just make no-public methods accessible.
The patch changes org.testng.internal.MethodInvocationHelper from:

70 boolean isPublic =
Modifier.isPublic(thisMethod.getModifiers());
71
72 try {
73 if (!isPublic) {
74 thisMethod.setAccessible(true);
75 }
76 result = thisMethod.invoke(instance, parameters);
77 }
78 finally {
79 if (!isPublic) {
80 thisMethod.setAccessible(false);
81 }
82 }
83
84 return result;

to:

70 synchronized (thisMethod) {
71 if (!Modifier.isPublic(thisMethod.getModifiers())) {
72 thisMethod.setAccessible(true);
73 }
74 }
75
76 return thisMethod.invoke(instance, parameters);

The corresponding patch is:

--- cbeust-testng-testng-6.0.1-0-gea2c4e7-1/cbeust-testng-ee25c93/src/
main/java/org/testng/internal/MethodInvocationHelper.java 2011-03-24
13:09:34.000000000 -0700
+++ cbeust-testng-testng-6.0.1-1-gea2c4e7-1/cbeust-testng-ee25c93/src/
main/java/org/testng/internal/MethodInvocationHelper.java 2011-06-22
10:18:16.000000000 -0700
@@ -67,21 +67,13 @@
}
}

- boolean isPublic = Modifier.isPublic(thisMethod.getModifiers());
-
- try {
- if (!isPublic) {
+ synchronized (thisMethod) {
+ if (!Modifier.isPublic(thisMethod.getModifiers())) {
thisMethod.setAccessible(true);
}
- result = thisMethod.invoke(instance, parameters);
- }
- finally {
- if (!isPublic) {
- thisMethod.setAccessible(false);
- }
}

- return result;
+ return thisMethod.invoke(instance, parameters);
}

protected static Iterator<Object[]> invokeDataProvider(Object
instance,

Cheers,
Baron

On Jun 14, 5:04 am, Baron <baron.robe...@gmail.com> wrote:
> Will do. I'm away from my keyboard this week but will report back
> early next week.
>
> Baron
>
> Cédric Beust ♔ wrote:
> > Hi Baron,
>

[Cédric Beust ♔]

Hi Baron,

I just implemented your fix in http://testng.org/beta , can you try it?

Thanks!

-- 

Cédric

[Baron]

Hi Cédric,

Thanks for putting in the fix. I have both reviewed the code change
and run the beta JAR against my tests. I am happy to report that all
are good!

Thanks,
Baron

On Jun 24, 11:16 am, Cédric Beust ♔ <ced...@beust.com> wrote:
> Hi Baron,
>

> I just implemented your fix inhttp://testng.org/beta, can you try it?
>
> Thanks!
>
> --
> Cédric