16f6c0aecf
Inline adoc files when they are included exactly once. Also fix language tags because this inlining gives us better information on what language the code is written in.
118 lines
3.6 KiB
Plaintext
118 lines
3.6 KiB
Plaintext
== Why is this an issue?
|
|
|
|
When a non-``++static++`` class field is annotated with ``++ThreadStatic++``, the code seems to show that the field can have different values for different calling threads, but that's not the case, since the ``++ThreadStatic++`` attribute is simply ignored on non-``++static++`` fields.
|
|
|
|
|
|
So ``++ThreadStatic++`` should either be removed or replaced with a use of the ``++ThreadLocal<T>++`` class, which gives a similar behavior for non-``++static++`` fields.
|
|
|
|
|
|
=== Noncompliant code example
|
|
|
|
[source,csharp]
|
|
----
|
|
public class MyClass
|
|
{
|
|
[ThreadStatic] // Noncompliant
|
|
private int count = 0;
|
|
|
|
// ...
|
|
}
|
|
----
|
|
|
|
|
|
=== Compliant solution
|
|
|
|
[source,csharp]
|
|
----
|
|
public class MyClass
|
|
{
|
|
private int count = 0;
|
|
|
|
// ...
|
|
}
|
|
----
|
|
or
|
|
|
|
[source,csharp]
|
|
----
|
|
public class MyClass
|
|
{
|
|
private readonly ThreadLocal<int> count = new ThreadLocal<int>();
|
|
public int Count
|
|
{
|
|
get { return count.Value; }
|
|
set { count.Value = value; }
|
|
}
|
|
// ...
|
|
}
|
|
----
|
|
|
|
|
|
|
|
ifdef::env-github,rspecator-view[]
|
|
|
|
'''
|
|
== Implementation Specification
|
|
(visible only on this page)
|
|
|
|
=== Message
|
|
|
|
Remove the "ThreadStatic" attribute from this definition.
|
|
|
|
|
|
'''
|
|
== Comments And Links
|
|
(visible only on this page)
|
|
|
|
=== on 8 Jun 2015, 08:43:39 Tamas Vajk wrote:
|
|
LGTM
|
|
|
|
=== on 15 Jun 2015, 12:39:35 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] Based on [~dinesh.bolkensteyn]'s comments I've changed the description a bit. Also, with this wording it is more like a bug than a maintainability issue. So I've modified the severity as well. I didn't change the SQALE characteristic, do you see any better option?
|
|
|
|
=== on 15 Jun 2015, 15:03:19 Ann Campbell wrote:
|
|
\[~tamas.vajk] as written, 'Critical' is not currently appropriate for this rule. If we're going to increase the severity, then the description needs to show why it's 'Critical'. What mistakes will this misunderstanding have lead the developer to make?
|
|
|
|
=== on 16 Jun 2015, 09:15:33 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] I've updated the description to be more bug-oriented.
|
|
|
|
=== on 16 Jun 2015, 11:17:19 Ann Campbell wrote:
|
|
\[~tamas.vajk] my 5 minutes with Google did not reveal the significance of ``++ThreadLocal++``. How is it relevant here?
|
|
|
|
=== on 17 Jun 2015, 07:16:29 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] I can understand that you couldn't find a lot of info on ``++ThreadLocal++``. It is only part of .Net 4, and it is probably rarely used.
|
|
|
|
|
|
If you have a ``++ThreadStatic++`` non-``++static++`` field, that behaves as a normal non-``++static++`` field. So the attribute is useless on it. You should remove it (first compliant solution). But what if you want a non-``++static++`` field that can store different values based on the thread we are using it from. Then you can use the ``++ThreadLocal++`` class (second complaint solution).
|
|
|
|
|
|
Check out the below code:
|
|
|
|
----
|
|
var m1 = new MyClass();
|
|
var m2 = new MyClass();
|
|
m1.Count = 5;
|
|
m2.Count = 7;
|
|
|
|
Task.Factory.StartNew(() =>
|
|
{
|
|
m1.Count = 6;
|
|
m2.Count = 8;
|
|
Console.WriteLine(m1.Count);
|
|
Console.WriteLine(m2.Count);
|
|
}).Wait();
|
|
|
|
Console.WriteLine(m1.Count);
|
|
Console.WriteLine(m2.Count);
|
|
----
|
|
|
|
It writes to the console ``++6,8,5,7++``. We have two instances of ``++MyClass++``, we set the ``++Count++`` to different values (the field is not static). Then start a new thread, and set the ``++Count++`` again to different values. In the new thread and in the main thread the ``++Count++``s have different values even for the same objects.
|
|
|
|
=== on 17 Jun 2015, 12:50:37 Ann Campbell wrote:
|
|
Okay, your turn [~tamas.vajk]. :-)
|
|
|
|
=== on 17 Jun 2015, 13:13:58 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] Thanks, it looks good, I'll run it through [~dinesh.bolkensteyn], and we'll see what he thinks.
|
|
|
|
endif::env-github,rspecator-view[]
|