diff --git a/rules/S2445/csharp/highlighting.adoc b/rules/S2445/csharp/highlighting.adoc new file mode 100644 index 00000000000..f48367061b0 --- /dev/null +++ b/rules/S2445/csharp/highlighting.adoc @@ -0,0 +1,4 @@ +=== Highlighting + +``lock (xxx)`` + diff --git a/rules/S2445/csharp/message.adoc b/rules/S2445/csharp/message.adoc index 7d74857bead..7d889534afb 100644 --- a/rules/S2445/csharp/message.adoc +++ b/rules/S2445/csharp/message.adoc @@ -4,7 +4,7 @@ "xxx" is a local variable, and should not be used for locking. -Use field from "yyy" for locking. +Use fields from "yyy" for locking. Locking on a new instance is a no-op. diff --git a/rules/S2445/csharp/rule-except-see.adoc b/rules/S2445/csharp/rule-except-see.adoc deleted file mode 100644 index f585e0a31bf..00000000000 --- a/rules/S2445/csharp/rule-except-see.adoc +++ /dev/null @@ -1,52 +0,0 @@ -Locking on a class field synchronizes not on the field itself, but on the object assigned to it. So locking on a non-``readonly`` field makes it possible for the field's value to change while a thread is in a block locked on the old value. That would allow a second thread, locked on the new value, to enter the block at the same time. - - -Locking on a ``readonly`` field of a class which is not ``private`` allows external code to lock the field, potentially interfering with synchronization of methods in that class. - - -Locking on a local variable or on a new instance undermines the synchronization: two different threads running the method in parallel would lock on two different object instances. - - -Locking on a string literal is even more dangerous: depending on whether the string is interned or not, different threads may or may not synchronize on the same object instance. - -== Noncompliant Code Example - -[source,csharp] ----- -private string color = "red"; - -private void DoSomething() -{ - lock (color) // Noncompliant; lock is actually on object instance "red" referred to by the color variable - { - //... - color = "green"; // other threads now allowed into this block - // ... - } - lock (new object()) // Noncompliant this is a no-op. - { - // ... - } -} ----- - - -== Compliant Solution - -[source,csharp] ----- -private string color = "red"; -private readonly object lockObj = new object(); - -private void DoSomething() -{ - lock (lockObj) - { - //... - color = "green"; - // ... - } -} ----- - - diff --git a/rules/S2445/csharp/rule.adoc b/rules/S2445/csharp/rule.adoc index 5d2f7852783..2abaaa44935 100644 --- a/rules/S2445/csharp/rule.adoc +++ b/rules/S2445/csharp/rule.adoc @@ -1,4 +1,60 @@ -include::rule-except-see.adoc[] +Locking on a class field synchronizes not on the field itself, but on the object assigned to it. So locking on a non-``readonly`` field makes it possible for the field's value to change while a thread is in a block locked on the old value. That would allow a second thread, locked on the new value, to enter the block at the same time. + + +Locking on a ``readonly`` field of a class which is not ``private`` allows external code to lock the field, potentially interfering with synchronization of methods in that class. + + +Locking on a local variable or on a new instance undermines the synchronization: two different threads running the same method in parallel would lock on two different object instances. That would allow a second thread, locked on that new value, to enter the same block concurrently. + + +Locking on a string literal is even more dangerous: depending on whether the string is interned or not, different threads may or may not synchronize on the same object instance. + + +== Noncompliant Code Example + +[source,csharp] +---- +private Color colorObject = new Color("red"); +private readonly colorString = "red"; + +private void DoSomething() +{ + // Synchronizing access via "colorObject" + lock (colorObject) // Noncompliant; lock is actually on object instance "red" referred to by the color field + { + //... + colorObject = new Color("green"); // other threads now allowed into this block + // ... + } + lock (new object()) // Noncompliant; this is a no-op + { + // ... + } + lock (colorString) // Noncompliant; strings can be interned + { + // ... + } +} +---- + + +== Compliant Solution + +[source,csharp] +---- +private Color colorObject = new Color("red"); +private readonly object lockObj = new object(); + +private void DoSomething() +{ + lock (lockObj) + { + //... + color = new Color("green"); + // ... + } +} +---- include::../see.adoc[] @@ -10,7 +66,7 @@ ifdef::env-github,rspecator-view[] include::message.adoc[] -include::../highlighting.adoc[] +include::highlighting.adoc[] ''' == Comments And Links diff --git a/rules/S2445/highlighting.adoc b/rules/S2445/java/highlighting.adoc similarity index 100% rename from rules/S2445/highlighting.adoc rename to rules/S2445/java/highlighting.adoc diff --git a/rules/S2445/message.adoc b/rules/S2445/java/message.adoc similarity index 100% rename from rules/S2445/message.adoc rename to rules/S2445/java/message.adoc diff --git a/rules/S2445/java/rule-except-see.adoc b/rules/S2445/java/rule-except-see.adoc deleted file mode 100644 index 719bdd5ed60..00000000000 --- a/rules/S2445/java/rule-except-see.adoc +++ /dev/null @@ -1,42 +0,0 @@ -Synchronizing on a class field synchronizes not on the field itself, but on the object assigned to it. So synchronizing on a non-``++final++`` field makes it possible for the field's value to change while a thread is in a block synchronized on the old value. That would allow a second thread, synchronized on the new value, to enter the block at the same time. - - -The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object instances in to the method as parameters, completely undermining the synchronization. - - -== Noncompliant Code Example - -[source,text] ----- -private String color = "red"; - -private void doSomething(){ - synchronized(color) { // Noncompliant; lock is actually on object instance "red" referred to by the color variable - //... - color = "green"; // other threads now allowed into this block - // ... - } - synchronized(new Object()) { // Noncompliant this is a no-op. - // ... - } -} ----- - - -== Compliant Solution - -[source,text] ----- -private String color = "red"; -private final Object lockObj = new Object(); - -private void doSomething(){ - synchronized(lockObj) { - //... - color = "green"; - // ... - } -} ----- - - diff --git a/rules/S2445/java/rule.adoc b/rules/S2445/java/rule.adoc index cb515687e5e..a36b0a3dafb 100644 --- a/rules/S2445/java/rule.adoc +++ b/rules/S2445/java/rule.adoc @@ -1,4 +1,43 @@ -include::rule-except-see.adoc[] +Synchronizing on a class field synchronizes not on the field itself, but on the object assigned to it. So synchronizing on a non-``++final++`` field makes it possible for the field's value to change while a thread is in a block synchronized on the old value. That would allow a second thread, synchronized on the new value, to enter the block at the same time. + + +The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object instances in to the method as parameters, completely undermining the synchronization. + + +== Noncompliant Code Example + +[source,text] +---- +private String color = "red"; + +private void doSomething(){ + synchronized(color) { // Noncompliant; lock is actually on object instance "red" referred to by the color variable + //... + color = "green"; // other threads now allowed into this block + // ... + } + synchronized(new Object()) { // Noncompliant this is a no-op. + // ... + } +} +---- + + +== Compliant Solution + +[source,text] +---- +private String color = "red"; +private final Object lockObj = new Object(); + +private void doSomething(){ + synchronized(lockObj) { + //... + color = "green"; + // ... + } +} +---- include::see.adoc[] @@ -10,7 +49,7 @@ ifdef::env-github,rspecator-view[] include::../message.adoc[] -include::../highlighting.adoc[] +include::highlighting.adoc[] ''' == Comments And Links