11

Is there any Checkstyle, PMD or Findbugs rule which could find the following non threadsafe spring singleton implementation?

private String helperVar;

public String getValue(String value) {
   helperVar = value;
   return convertValue();
}

private String convertValue() {
   return helperVar.trim();
}

I know this sample is horrible but it is the easiest way to show what I mean.

When executing the getValue method from the bean in a single execution it would work fine. But when executing it in a multi user environment it would lead to unpredictable errors/behaviour.

Is there a way to find these occurences without going through the code manually? Are there any static code checkers which could check this and each variation of it automatically?

B4dT0bi
  • 623
  • 4
  • 21
  • A field's setter and `toString` based on that field, called consecutively, already would suffice. Creeps. – Joop Eggen Jan 17 '14 at 15:27
  • 1
    thanks for trying to help me but I dont have issues with the code I just want to find other occurences of the same pattern automatically, So there is no question related to the sample code. The question is solely about the code checkers available. – B4dT0bi Jan 17 '14 at 15:30
  • 1
    I did not criticize the code, but wanted to indicate that your code pattern, _"changing a field and calculating with several occurrences of that field"_, is very common. – Joop Eggen Jan 17 '14 at 18:52
  • 1
    but the question was how to find these occurences with the help of automated tools – B4dT0bi Jan 18 '14 at 00:49
  • This particular case is tough. There is the FindBugs rule [IS_FIELD_NOT_GUARDED](http://findbugs.sourceforge.net/bugDescriptions.html#IS_FIELD_NOT_GUARDED), which is supposed to do that. But it works only if the field is annotated with [`@GuardedBy`](http://jcip.net.s3-website-us-east-1.amazonaws.com/annotations/doc/net/jcip/annotations/GuardedBy.html), which will be difficult for you to ensure. Also, in my test, it simply didn't work. :-( – barfuin Jan 18 '14 at 19:09
  • You realize that this is not a bug *unless* this class is supposed to be threadsafe. FindBugs, for instance, treats all classes as "not threadsafe" unless they are annotated as being threadsafe. So you will either get loads of false positives or you will have to annotate the threadsafe classes with [`@ThreadSafe`](http://jcip.net.s3-website-us-east-1.amazonaws.com/annotations/doc/net/jcip/annotations/ThreadSafe.html). – barfuin Jan 23 '14 at 20:03

2 Answers2

1

This might not be acceptable to you but I sometimes employ runtime bean reflection to validate code consistency.

For your use case I would first make all of my beans use constructor based injection and make all member fields final. I believe findbugs even has some immutable bean checkers.

Second to do the code consistency for your use case I would use either Spring BeanPostProcessor or just a class that implements ApplicationContextAware and then walks the ApplicationContext. Now you just check the beans that get loaded in the application context that are yours (just check the package name of the beans class) to make sure that all the fields are final. Yes you will need a more lax security manager or enable your security policy to allow private variable reflection but for most this is not a problem especially if your already using something like hibernate.

If there is a field that is not final ie invalid code you just throw an exception and your Spring app will not start.

For various exceptions to the rule you could use custom annotations for fields that need not be final or classes thats need to ignore the rule.

You might be concerned with performance or that Spring constructor injection is not powerful enough but Spring already does an enormous amount of reflection anyway on boot up and constructor based injection has gotten rather powerful these days such that you can even do property place holder values with @Value(${PROP}).

Adam Gent
  • 47,843
  • 23
  • 153
  • 203
1

You can try with

FindBugs library

It has some build in methods.

There is no exactly what you need but it can be extended for your needs. This is good example

Daniel Schneller blog

The general idea of a detector is to find certain patterns in the bytecode of a class. To do so it will read the .class files and put them through pattern matchers that are implemented using the visitor pattern. While reading it will call appropriate visitor methods, depending on what element (method, field declaration etc.) is at hand. Writing a detector means implementing one or more of those methods, building up an idea about what the class is intended to do

Good thing is that on the end it is Java code you can include in your test module. I think you can also add compiler warning but not sure about that.

mommcilo
  • 956
  • 11
  • 28