added check to detect the double-checked locking idiom, rfe #709333
This commit is contained in:
parent
ba1663fc94
commit
8f965910b0
|
|
@ -70,6 +70,9 @@
|
|||
<li>
|
||||
<a href="#FinalParameters">FinalParameters</a>
|
||||
</li>
|
||||
<li>
|
||||
<a href="#DoubleCheckedLocking">DoubleCheckedLocking</a>
|
||||
</li>
|
||||
</ul>
|
||||
</td>
|
||||
<!--Content-->
|
||||
|
|
@ -787,6 +790,52 @@ public class StringUtils // not final to allow subclassing
|
|||
<a href="config.html#treewalker">TreeWalker</a>
|
||||
</p>
|
||||
|
||||
<!-- --> <a name="DoubleCheckedLocking"></a> <h2>DoubleCheckedLocking</h2> <h4>Description</h4>
|
||||
<p class="body">
|
||||
The "double-checked locking" idiom (DCL) tries to avoid the runtime cost
|
||||
of synchronization. An example that uses the DCL idiom is this:
|
||||
</p>
|
||||
<pre class="body">
|
||||
public class MySingleton
|
||||
{
|
||||
private theInstance = null;
|
||||
|
||||
private MySingleton() {}
|
||||
|
||||
public MySingleton getInstance() {
|
||||
if ( theInstance == null ) { // synchronize only if necessary
|
||||
synchronized( MySingleton.class ) {
|
||||
if ( s_singleton == null ) {
|
||||
theInstance = new MySingleton();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
</pre>
|
||||
<p class="body">
|
||||
The problem with the DCL idiom in Java is that it just does not work correctly.
|
||||
Using it introduces bugs that are extremely hard to track down and reproduce.
|
||||
The <a href="http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html">
|
||||
"Double-Checked Locking is Broken" Declaration</a> has an in depth explanation
|
||||
of the exact problem which has to do with the semantics of the Java memory model.
|
||||
</p>
|
||||
<h4>Examples</h4>
|
||||
<p class="body">
|
||||
To configure the check:
|
||||
</p>
|
||||
<pre class="body">
|
||||
<module name="DoubleCheckedLocking"/>
|
||||
</pre>
|
||||
<h4>Package</h4>
|
||||
<p class="body">
|
||||
com.puppycrawl.tools.checkstyle.checks
|
||||
</p>
|
||||
<h4>Parent Module</h4>
|
||||
<p class="body">
|
||||
<a href="config.html#treewalker">TreeWalker</a>
|
||||
</p>
|
||||
|
||||
</td>
|
||||
</tr>
|
||||
</table>
|
||||
|
|
|
|||
|
|
@ -76,6 +76,9 @@
|
|||
[Bloch, Effective Java], Item 17 "Use interfaces only to define
|
||||
types").</li>
|
||||
|
||||
<li class="body">Added check to detect the double-checked locking
|
||||
idiom (module DoubleCheckedLocking, request 709333).</li>
|
||||
|
||||
<li class="body">Added check to enforce C style (char c[]) or Java
|
||||
style (char[] c) for array type declaration (module ArrayTypeStyle,
|
||||
request 493380).</li>
|
||||
|
|
|
|||
|
|
@ -0,0 +1,93 @@
|
|||
////////////////////////////////////////////////////////////////////////////////
|
||||
// checkstyle: Checks Java source code for adherence to a set of rules.
|
||||
// Copyright (C) 2001-2002 Oliver Burn
|
||||
//
|
||||
// This library is free software; you can redistribute it and/or
|
||||
// modify it under the terms of the GNU Lesser General Public
|
||||
// License as published by the Free Software Foundation; either
|
||||
// version 2.1 of the License, or (at your option) any later version.
|
||||
//
|
||||
// This library is distributed in the hope that it will be useful,
|
||||
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
||||
// Lesser General Public License for more details.
|
||||
//
|
||||
// You should have received a copy of the GNU Lesser General Public
|
||||
// License along with this library; if not, write to the Free Software
|
||||
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
|
||||
////////////////////////////////////////////////////////////////////////////////
|
||||
package com.puppycrawl.tools.checkstyle.checks;
|
||||
|
||||
import antlr.collections.AST;
|
||||
import com.puppycrawl.tools.checkstyle.api.Check;
|
||||
import com.puppycrawl.tools.checkstyle.api.DetailAST;
|
||||
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
|
||||
|
||||
/**
|
||||
* Detect the double-checked locking idiom, a technique that tries to avoid
|
||||
* synchronization overhead but is incorrect because of subtle artifacts of
|
||||
* the java memory model.
|
||||
*
|
||||
* See <a href=
|
||||
* "http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html"
|
||||
* >The "Double-Checked Locking is Broken" Declaration</a>
|
||||
* for a more in depth explanation.
|
||||
*
|
||||
* @author Lars Kühne
|
||||
*/
|
||||
public class DoubleCheckedLockingCheck extends Check
|
||||
{
|
||||
/** @see Check */
|
||||
public int[] getDefaultTokens()
|
||||
{
|
||||
return new int[]{TokenTypes.LITERAL_IF};
|
||||
}
|
||||
|
||||
/** @see Check */
|
||||
public void visitToken(DetailAST aAST)
|
||||
{
|
||||
DetailAST synchronizedAST =
|
||||
getLowestParent(aAST, TokenTypes.LITERAL_SYNCHRONIZED);
|
||||
if (synchronizedAST == null) {
|
||||
return;
|
||||
}
|
||||
|
||||
DetailAST ifAST =
|
||||
getLowestParent(synchronizedAST, TokenTypes.LITERAL_IF);
|
||||
if (ifAST == null) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (getIfCondition(aAST).equalsTree(getIfCondition(ifAST))) {
|
||||
log(aAST.getLineNo(), aAST.getColumnNo(),
|
||||
"doublechecked.locking.avoid");
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* returns the condition of an if statement.
|
||||
* @param aIfAST the LITERAL_IF AST
|
||||
* @return the AST that represents the condition of the if statement
|
||||
*/
|
||||
private AST getIfCondition(DetailAST aIfAST)
|
||||
{
|
||||
return aIfAST.getFirstChild().getNextSibling();
|
||||
}
|
||||
|
||||
/**
|
||||
* searches towards the root of the AST for a specific AST type.
|
||||
* @param aAST the starting node for searching (inclusive)
|
||||
* @param aTokenType the token type to search for
|
||||
* @return the first token of type aTokenTye or null if no such token exists
|
||||
*/
|
||||
private DetailAST getLowestParent(DetailAST aAST, int aTokenType)
|
||||
{
|
||||
DetailAST synchronizedParent = aAST;
|
||||
while (synchronizedParent != null
|
||||
&& synchronizedParent.getType() != aTokenType)
|
||||
{
|
||||
synchronizedParent = synchronizedParent.getParent();
|
||||
}
|
||||
return synchronizedParent;
|
||||
}
|
||||
}
|
||||
|
|
@ -72,3 +72,5 @@ translation.missingKey=Key ''{0}'' missing.
|
|||
|
||||
simplify.expression=Expression can be simplified.
|
||||
simplify.boolreturn=Conditional logic can be removed.
|
||||
|
||||
doublechecked.locking.avoid=The double-checked locking idiom is broken and should be avoided.
|
||||
|
|
|
|||
|
|
@ -73,3 +73,5 @@ translation.missingKey=
|
|||
|
||||
simplify.expression=Der Ausdruck kann vereinfacht werden.
|
||||
simplify.boolreturn=Die Verzweigung sollte entfernt werden.
|
||||
|
||||
doublechecked.locking.avoid=Das ''double-checked locking'' Idiom sollte vermieden werden.
|
||||
|
|
|
|||
|
|
@ -72,3 +72,5 @@ translation.missingKey=Käännösavain ''{0}'' puuttuu.
|
|||
|
||||
simplify.expression=Ilmaisua voisi yksinkertaistaa.
|
||||
simplify.boolreturn=Konditionaalilogiikan voisi poistaa.
|
||||
|
||||
doublechecked.locking.avoid=The double-checked locking idiom is broken and should be avoided.
|
||||
|
|
|
|||
|
|
@ -72,3 +72,5 @@ translation.missingKey=Clef ''{0}'' manquante.
|
|||
|
||||
simplify.expression=L'expression peut-être simplifiée.
|
||||
simplify.boolreturn=Supprimez la logique conditionnelle.
|
||||
|
||||
doublechecked.locking.avoid=The double-checked locking idiom is broken and should be avoided.
|
||||
|
|
|
|||
|
|
@ -72,3 +72,5 @@ translation.missingKey=Falta a chave ''{0}''.
|
|||
|
||||
simplify.expression=Expressão pode ser simplicada.
|
||||
simplify.boolreturn=A lógica condicional deve ser removida.
|
||||
|
||||
doublechecked.locking.avoid=The double-checked locking idiom is broken and should be avoided.
|
||||
|
|
|
|||
|
|
@ -55,6 +55,7 @@ import com.puppycrawl.tools.checkstyle.checks.UpperEllCheckTest;
|
|||
import com.puppycrawl.tools.checkstyle.checks.VisibilityModifierCheckTest;
|
||||
import com.puppycrawl.tools.checkstyle.checks.WhitespaceAfterCheckTest;
|
||||
import com.puppycrawl.tools.checkstyle.checks.WhitespaceAroundTest;
|
||||
import com.puppycrawl.tools.checkstyle.checks.DoubleCheckedLockingCheckTest;
|
||||
import junit.framework.Test;
|
||||
import junit.framework.TestSuite;
|
||||
|
||||
|
|
@ -78,6 +79,7 @@ public class AllTests {
|
|||
suite.addTest(new TestSuite(AvoidStarImportTest.class));
|
||||
suite.addTest(new TestSuite(ConfigurationLoaderTest.class));
|
||||
suite.addTest(new TestSuite(ConstantNameCheckTest.class));
|
||||
suite.addTest(new TestSuite(DoubleCheckedLockingCheckTest.class));
|
||||
suite.addTest(new TestSuite(EmptyBlockCheckTest.class));
|
||||
suite.addTest(new TestSuite(EmptyForIteratorPadCheckTest.class));
|
||||
suite.addTest(new TestSuite(EqualsHashCodeCheckTest.class));
|
||||
|
|
|
|||
|
|
@ -0,0 +1,22 @@
|
|||
package com.puppycrawl.tools.checkstyle.checks;
|
||||
|
||||
import com.puppycrawl.tools.checkstyle.BaseCheckTestCase;
|
||||
import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
|
||||
|
||||
/**
|
||||
* @author lkuehne
|
||||
*/
|
||||
public class DoubleCheckedLockingCheckTest
|
||||
extends BaseCheckTestCase
|
||||
{
|
||||
public void testIt() throws Exception
|
||||
{
|
||||
final DefaultConfiguration checkConfig =
|
||||
createCheckConfig(DoubleCheckedLockingCheck.class);
|
||||
final String[] expected = {
|
||||
"34:17: The double-checked locking idiom is broken and should be avoided.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputDoubleCheckedLocking.java"), expected);
|
||||
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue