You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2011/09/19 18:30:36 UTC
svn commit: r1172689 -
/tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
Author: markt
Date: Mon Sep 19 16:30:36 2011
New Revision: 1172689
URL: http://svn.apache.org/viewvc?rev=1172689&view=rev
Log:
Fix threading issue with changing visibility of methods and fields
Modified:
tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
Modified: tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java?rev=1172689&r1=1172688&r2=1172689&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java Mon Sep 19 16:30:36 2011
@@ -187,9 +187,11 @@ public class DefaultInstanceManager impl
if (entry.getType() == AnnotationCacheEntryType.POST_CONSTRUCT) {
Method postConstruct = (Method) entry.getAccessibleObject();
boolean accessibility = postConstruct.isAccessible();
- postConstruct.setAccessible(true);
- postConstruct.invoke(instance);
- postConstruct.setAccessible(accessibility);
+ synchronized (postConstruct) {
+ postConstruct.setAccessible(true);
+ postConstruct.invoke(instance);
+ postConstruct.setAccessible(accessibility);
+ }
}
}
}
@@ -229,9 +231,11 @@ public class DefaultInstanceManager impl
if (entry.getType() == AnnotationCacheEntryType.PRE_DESTROY) {
Method preDestroy = (Method) entry.getAccessibleObject();
boolean accessibility = preDestroy.isAccessible();
- preDestroy.setAccessible(true);
- preDestroy.invoke(instance);
- preDestroy.setAccessible(accessibility);
+ synchronized (preDestroy) {
+ preDestroy.setAccessible(true);
+ preDestroy.invoke(instance);
+ preDestroy.setAccessible(accessibility);
+ }
}
}
}
@@ -572,9 +576,11 @@ public class DefaultInstanceManager impl
}
accessibility = field.isAccessible();
- field.setAccessible(true);
- field.set(instance, lookedupResource);
- field.setAccessible(accessibility);
+ synchronized (field) {
+ field.setAccessible(true);
+ field.set(instance, lookedupResource);
+ field.setAccessible(accessibility);
+ }
}
/**
@@ -614,9 +620,11 @@ public class DefaultInstanceManager impl
}
accessibility = method.isAccessible();
- method.setAccessible(true);
- method.invoke(instance, lookedupResource);
- method.setAccessible(accessibility);
+ synchronized (method) {
+ method.setAccessible(true);
+ method.invoke(instance, lookedupResource);
+ method.setAccessible(accessibility);
+ }
}
public static String getName(Method setter) {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1172689 - /tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/9/20 sebb <se...@gmail.com>:
> On 19 September 2011 17:30, <ma...@apache.org> wrote:
>> Author: markt
>> Date: Mon Sep 19 16:30:36 2011
>> New Revision: 1172689
>>
>> URL: http://svn.apache.org/viewvc?rev=1172689&view=rev
>> Log:
>> Fix threading issue with changing visibility of methods and fields
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
>>
>> Modified: tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java?rev=1172689&r1=1172688&r2=1172689&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java (original)
>> +++ tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java Mon Sep 19 16:30:36 2011
>> @@ -187,9 +187,11 @@ public class DefaultInstanceManager impl
>> if (entry.getType() == AnnotationCacheEntryType.POST_CONSTRUCT) {
>> Method postConstruct = (Method) entry.getAccessibleObject();
>> boolean accessibility = postConstruct.isAccessible();
>> - postConstruct.setAccessible(true);
>> - postConstruct.invoke(instance);
>> - postConstruct.setAccessible(accessibility);
>> + synchronized (postConstruct) {
>> + postConstruct.setAccessible(true);
>> + postConstruct.invoke(instance);
>> + postConstruct.setAccessible(accessibility);
>> + }
>
> Could skip the synch. block and two method calls if the field is
> already accessible:
>
> if (accessibility) {
> postConstruct.invoke(instance);
> } else {
> synchronized (postConstruct) {
> postConstruct.setAccessible(true);
> postConstruct.invoke(instance);
> postConstruct.setAccessible(accessibility);
> }
> }
>
> Similarly for the other synch. blocks below.
>
My understanding is that "isAccessible()" is false by default for a
new instance of Method object (regardless of whether the method and
class are public), and so your proposed optimization actually will
never work.
My thought is that setAccessible(true); can be called once - when the
annotation is detected and placed into the cache.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1172689 - /tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
Posted by sebb <se...@gmail.com>.
On 19 September 2011 17:30, <ma...@apache.org> wrote:
> Author: markt
> Date: Mon Sep 19 16:30:36 2011
> New Revision: 1172689
>
> URL: http://svn.apache.org/viewvc?rev=1172689&view=rev
> Log:
> Fix threading issue with changing visibility of methods and fields
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
>
> Modified: tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java?rev=1172689&r1=1172688&r2=1172689&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/core/DefaultInstanceManager.java Mon Sep 19 16:30:36 2011
> @@ -187,9 +187,11 @@ public class DefaultInstanceManager impl
> if (entry.getType() == AnnotationCacheEntryType.POST_CONSTRUCT) {
> Method postConstruct = (Method) entry.getAccessibleObject();
> boolean accessibility = postConstruct.isAccessible();
> - postConstruct.setAccessible(true);
> - postConstruct.invoke(instance);
> - postConstruct.setAccessible(accessibility);
> + synchronized (postConstruct) {
> + postConstruct.setAccessible(true);
> + postConstruct.invoke(instance);
> + postConstruct.setAccessible(accessibility);
> + }
Could skip the synch. block and two method calls if the field is
already accessible:
if (accessibility) {
postConstruct.invoke(instance);
} else {
synchronized (postConstruct) {
postConstruct.setAccessible(true);
postConstruct.invoke(instance);
postConstruct.setAccessible(accessibility);
}
}
Similarly for the other synch. blocks below.
> }
> }
> }
> @@ -229,9 +231,11 @@ public class DefaultInstanceManager impl
> if (entry.getType() == AnnotationCacheEntryType.PRE_DESTROY) {
> Method preDestroy = (Method) entry.getAccessibleObject();
> boolean accessibility = preDestroy.isAccessible();
> - preDestroy.setAccessible(true);
> - preDestroy.invoke(instance);
> - preDestroy.setAccessible(accessibility);
> + synchronized (preDestroy) {
> + preDestroy.setAccessible(true);
> + preDestroy.invoke(instance);
> + preDestroy.setAccessible(accessibility);
> + }
> }
> }
> }
> @@ -572,9 +576,11 @@ public class DefaultInstanceManager impl
> }
>
> accessibility = field.isAccessible();
> - field.setAccessible(true);
> - field.set(instance, lookedupResource);
> - field.setAccessible(accessibility);
> + synchronized (field) {
> + field.setAccessible(true);
> + field.set(instance, lookedupResource);
> + field.setAccessible(accessibility);
> + }
> }
>
> /**
> @@ -614,9 +620,11 @@ public class DefaultInstanceManager impl
> }
>
> accessibility = method.isAccessible();
> - method.setAccessible(true);
> - method.invoke(instance, lookedupResource);
> - method.setAccessible(accessibility);
> + synchronized (method) {
> + method.setAccessible(true);
> + method.invoke(instance, lookedupResource);
> + method.setAccessible(accessibility);
> + }
> }
>
> public static String getName(Method setter) {
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org