5

These 3 types of lock are apparently bad. What other type of locking is bad? Are there Stylecop / FxCop rules that would catch this? If not, then would you please help me with a custom rule implementation? They code for all of them must be similar, right?

Thank you.

Randy Levy
  • 22,566
  • 4
  • 68
  • 94
Hamish Grubijan
  • 10,562
  • 23
  • 99
  • 147
  • 1
    StyleCop only checks coding style and not behavior. If you need a rule for this, FxCop is the tool for this. – Steven May 31 '10 at 20:28
  • Dumb question: when I right-click a C# project in VS2010 and select 'Run Code Analysis" ... is that FxCop, or some other tool? Also: if I want to detect a case when an exception is re-thrown and a stack trace has been cut-off - is that also a job of FxCop? – Hamish Grubijan May 31 '10 at 20:36
  • Odd, you keep asking for tools that cannot outperform an editor's Find capability. – Hans Passant May 31 '10 at 21:57
  • 1
    :) Hans, the benefit of a tool multiplies 20-fold in a team environment. – Hamish Grubijan Jun 01 '10 at 02:21
  • 1
    Right click -> Code Analysis is FXCop, well, the built in FXCop. – blowdart Jun 01 '10 at 04:48

2 Answers2

3

The samples (you may need to allow popups in your browser) of John Robbins' Debugging Microsoft .NET Applications book contain sources for such FxCop rules (DoNotLockOnPublicFields, DoNotLockOnThisOrMe, DoNotLockOnTypes, etc.). It looks like they were originally made for FxCop 1.35, whereas the version in VS 2008 and the latest standalone version is 1.36 (not to speak of VS2010). So they might need some tweaking, YMMV.

There is also rule CA2002 (Do not lock on objects with weak identity), which checks for stuff like lock(typeof(...)), but not for lock(this)

Christian.K
  • 47,778
  • 10
  • 99
  • 143
1

Basically, you should not lock on any external object unless this is specifically a locking object (such as the SyncRoot property on the non-generic ICollection was designed for). Doing so carries the risk of other "users" of the reference also locking on it, leading to unwanted locking or even deadlocks.

Obvisously, this and typeof() are by definition external objects. Strings are immutable and string literals are all interned, so that the same reference can be in unse at different places even if you directly assigned it in your object.

I don't know of a StyleCop rule for those, but I don't have a good overview of what is available for StyleCop or FxCop, so there could very well be something in the wild to check for those cases. I'd check for synching only on private members which are not strings and not directly returned in any property or method.

Lucero
  • 59,176
  • 9
  • 122
  • 152
  • By the way, strings are immutable, but the CLR also never generates more than one literal constant with the same value, e.g: `const string l1 = "lock";` and `const string l2 = "lock";` can trip one up when `l1` and `l2` are both used inside a lock statement. – Hamish Grubijan May 31 '10 at 20:25
  • 1
    And strings can even be shared between AppDomains, increasing the chance on deadlocks even further. – Steven May 31 '10 at 20:27
  • Nice, Steven ... seems like the `lock` feature of .Net could have been designed in a noob-friendlier way (in a hindsight). I hope others will point out more common dangerous usages of `lock`. – Hamish Grubijan May 31 '10 at 20:34