groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jochen Theodorou <blackd...@gmx.org>
Subject Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of "static" fields should be "synchronized"
Date Wed, 06 Jan 2016 09:09:39 GMT
nono, iot is actually good to question things. Having two people review 
things is good, and I must say, I think I rushed a bit here and did 
misread the code (too much task switching and too little time for Groovy 
these days I assume).

So what is myMetaClass for? When I did the review I assumed it is for 
the object we "handle". But that is not the case. Instead this is really 
the meta class for the handle itself, used by the handle itself.

What does it mean then to have this static? Subclasses will not 
influence that and there won't be any per instance meta classes for 
this. Now I think it would be no good to have a per instance meta class 
on a handle meta class... I mean a meta class of a meta class... well...

In fact I now think we should do a different change. If we decide to 
have this static, then I guess the class should actually be final.

And then I would suggest writing a static method to get this meta class, 
which does also do the init and everything wanting to work with the 
myMetaClass should use this method then. This actually moves the init of 
the meta class to an even later point than it is right now, but that is 
fine.

As for concurrency... I think if done this way it is no problem. 
Multiple threads might create multiple meta classes and they are 
different instances and all, but the will do the same, so in the end it 
does not matter which of those is used in the end. The meta class itself 
is responsible for proper synchronization and initializaton of anything 
that is visible across threads, so no external synchronization is required.

what do you guys think?

bye Jochen

Am 06.01.2016 um 00:27 schrieb John Wagenleitner:
> My mistake, I didn't know there was a PR associated that Jochen had
> already reviewed.  Sorry about that.
>
> On Tue, Jan 5, 2016 at 2:45 PM, Pascal Schumacher
> <pascalschumacher@gmx.net <mailto:pascalschumacher@gmx.net>> wrote:
>
>     Yes, I agree the change most likely reduces performance, but
>     increases thread-safety  (prevent that different Threads may work
>     with different Metaclasses etc.)
>
>     I do not know enough about this area of the code to judge if the
>     lack of thread-safety is really a concern.
>
>     I just merged the pull request because of Jochens +1 vote.
>
>
>     Am 05.01.2016 um 20:31 schrieb John Wagenleitner:
>>     Not sure but wonder if HandleMetaClass#myMetaClass was static to
>>     avoid having to perform a lookup for each HMC instance.  Probably
>>     not a issue but thought I'd bring attention to it.
>>
>>     On Tue, Jan 5, 2016 at 10:13 AM, <pascalschumacher@apache.org
>>     <mailto:pascalschumacher@apache.org>> wrote:
>>
>>         Repository: groovy
>>         Updated Branches:
>>           refs/heads/master 586a316da -> c5f17abbe
>>
>>
>>         Fixing squid:S2444 - Lazy initialization of "static" fields
>>         should be "synchronized"
>>
>>         <snip>
>>
>>         http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>         ----------------------------------------------------------------------
>>         diff --git
>>         a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>         b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>         index f421131..9167f5c 100644
>>         --- a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>         +++ b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>         @@ -24,7 +24,7 @@ import java.lang.reflect.Method;
>>
>>          public class HandleMetaClass extends DelegatingMetaClass {
>>              private Object object;
>>         -    private static MetaClass myMetaClass;
>>         +    private MetaClass myMetaClass;
>>              private static final Object NONE = new Object();
>>
>>              public HandleMetaClass(MetaClass mc) {
>>
>>         <snip>
>>
>
>

Mime
View raw message