Prev: toolstrip - mouse event for disabled items?
Next: Calling Internal Method using Reflection
From: za on 6 Jun 2010 06:43
Hello,

I get a CA2000 warning on the following code.

public class Taskbar
{
private static Taskbar _default;

private bool _isSynchronized;

public static Taskbar Default
{
if (_default == null)
{
_default = Taskbar.Synchronized(new Taskbar());
}

return _default;
}

public static Taskbar Synchronized(Taskbar taskbar)
{
taskbar._isSynchronized = true;
return taskbar;
}
}

Can somebody explain me how to resolve the CA2000 warnings on the code?

Regards,

Fred


From: Peter Duniho on 6 Jun 2010 16:44
za wrote:
> Hello,
>
> I get a CA2000 warning on the following code.
>
> public class Taskbar
> {
> private static Taskbar _default;
>
> private bool _isSynchronized;
>
> public static Taskbar Default
> {
> if (_default == null)
> {
> _default = Taskbar.Synchronized(new Taskbar());
> }
>
> return _default;
> }
>
> public static Taskbar Synchronized(Taskbar taskbar)
> {
> taskbar._isSynchronized = true;
> return taskbar;
> }
> }
>
> Can somebody explain me how to resolve the CA2000 warnings on the code?

There's not enough context to know for sure what an appropriate
resolution would be. Especially since the code you did post doesn't
compile, never mind include any types that implement IDisposable.

But in general, you'll get that warning when it appears that there's a
way for an object implementing IDisposable to not be visible after a
method returns, but on which the Dispose() method has not been called
before the method returns.

Post a concise-but-complete code example that reliably reproduces the
warning if you want more specific advice.

Finally, note that it is a good idea to make static members of a type
thread-safe, even if instances of the class are not intended to be
thread-safe. This is especially true for singletons and singleton-like
members, such as your "Default" member.

Pete
From: za on 7 Jun 2010 03:34

"Peter Duniho" <NpOeStPeAdM(a)NnOwSlPiAnMk.com> wrote in message
news:dcydndXHMpI2lZHRnZ2dnUVZ_hudnZ2d(a)posted.palinacquisition...
> za wrote:
>> Hello,
>>
>> I get a CA2000 warning on the following code.
>>
>> public class Taskbar
>> {
>> private static Taskbar _default;
>>
>> private bool _isSynchronized;
>>
>> public static Taskbar Default
>> {
>> if (_default == null)
>> {
>> _default = Taskbar.Synchronized(new Taskbar());
>> }
>>
>> return _default;
>> }
>>
>> public static Taskbar Synchronized(Taskbar taskbar)
>> {
>> taskbar._isSynchronized = true;
>> return taskbar;
>> }
>> }
>>
>> Can somebody explain me how to resolve the CA2000 warnings on the code?
>
> There's not enough context to know for sure what an appropriate resolution
> would be. Especially since the code you did post doesn't compile, never
> mind include any types that implement IDisposable.
>
> But in general, you'll get that warning when it appears that there's a way
> for an object implementing IDisposable to not be visible after a method
> returns, but on which the Dispose() method has not been called before the
> method returns.
>
> Post a concise-but-complete code example that reliably reproduces the
> warning if you want more specific advice.
>
> Finally, note that it is a good idea to make static members of a type
> thread-safe, even if instances of the class are not intended to be
> thread-safe. This is especially true for singletons and singleton-like
> members, such as your "Default" member.
>
> Pete

Hi Pete,

using System;
using System.Configuration;

namespace CA2000
{
internal class Settings1 : ApplicationSettingsBase, IDisposable
{
private static Settings1 defaultInstance =
((Settings1)(ApplicationSettingsBase.Synchronized(new Settings1())));

public static Settings1 Default
{
get
{
return defaultInstance;
}
}

public void Dispose()
{

}
}
}

Maybe this is the simplest sample to reproduce the CA2000 error.

Regards,

Fred


From: Peter Duniho on 7 Jun 2010 04:59
za wrote:
> using System;
> using System.Configuration;
>
> namespace CA2000
> {
> internal class Settings1 : ApplicationSettingsBase, IDisposable
> {
> private static Settings1 defaultInstance =
> ((Settings1)(ApplicationSettingsBase.Synchronized(new Settings1())));
>
> public static Settings1 Default
> {
> get
> {
> return defaultInstance;
> }
> }
>
> public void Dispose()
> {
>
> }
> }
> }
>
> Maybe this is the simplest sample to reproduce the CA2000 error.

Well, actually…the above example doesn't need the Default property in
order to reproduce the warning. That said, it's a good example, and
provides plenty to work with in terms of answering the question.

Unfortunately, I'm not there's a good _answer_. :)

As you're probably already aware, the bottom line issue here is that an
object is allocated, and then as far as the code analysis can tell, is
not disposed of before you no longer have a reference to it.

Of course, the object returned by the Synchronized() method in fact
refers to the original object (indeed, _is_ the original object), and so
the reference still exists, and so doesn't need to be disposed (and in
fact, cannot be). But there's no direct way for the code analyzer to
know that. It can't make assumptions about what the Synchronized()
method does, and it doesn't do any sort of called-method inspection to
make its determinations.

As far as potential resolutions go, here is a version of the code that
doesn't cause the warning:

using System;
using System.Configuration;

namespace CA2000
{
internal class Settings1 : ApplicationSettingsBase, IDisposable
{
private static Settings1 defaultInstance;

static Settings1()
{
defaultInstance = new Settings1();

defaultInstance =
(Settings1)ApplicationSettingsBase.Synchronized(defaultInstance);
}

public void Dispose()
{

}
}
}

Note that by writing the static constructor explicitly, _and_ by
assigning the original Settings1() object to your field before passing
it to the Synchronized() method, that's enough to convince code analysis
that you've let the object reference escape the constructor, and thus it
does not need to be disposed of before the constructor returns.

I find this resolution unfortunate in two ways:

• First, it would be nice if there were some way to explain to the
analyzer that in this case, passing the object reference to the
Synchronized() method does in fact preserve a reference to the object
after the constructor returns.

• Second, it concerns me that the code analysis doesn't pick up on
the fact that the "defaultInstance" field is being overwritten, which
absent any specific knowledge of the Synchronized() method (which
clearly the analyzer does not have) _should_ in fact mean that the
original object reference has been lost before the object has been disposed.

I find this second issue especially ironic, because it appears that when
the variable is a local variable (i.e. if you try to move the
initialization into a helper method with a try/catch to deal with
disposing the original object on error), the analyzer _does_ in fact
pick up on the fact that it's been overwritten, and so returning the new
value does not satisfy the analyzer with respect to the original value.

I don't really see a good way around the first issue, other than just
suppressing the warning (which is a blunt way to give the analyzer that
information). The second issue concerns me because it's possible
someone at Microsoft will eventually decide it's a bug that the analyzer
doesn't figure that out and will fix the bug, and then my proposed
work-around isn't going to help.

I think given all the above, personally I'd just suppress the warning
and be done with it. The fact is, it's a situation that given the kind
of analysis used for that rule, I think really cannot be detected as
safe by the analyzer, so you're left helping it out by suppressing the
warning explicitly.

Pete
From: Peter Duniho on 7 Jun 2010 05:32
Peter Duniho wrote:
> [...]
> As far as potential resolutions go, here is a version of the code that
> doesn't cause the warning: [...]

Here's a different approach that also avoids the warning:

using System;
using System.Configuration;

namespace CA2000
{
internal class Settings1 : ApplicationSettingsBase, IDisposable
{
private static Settings1 defaultInstance = new Settings1();

public static Settings1 Default
{
get
{
if (!defaultInstance.IsSynchronized)
{
defaultInstance =
(Settings1)SettingsBase.Synchronized(defaultInstance);
}

return defaultInstance;
}
}

public void Dispose()
{

}
}
}

It requires the use of the getter, but given your previous example, I'm
assuming that's not actually a problem.

Unfortunately, it's somewhat "hackish" also. In particular, it doesn't
require synchronization in the getter only because this solution assumes
knowledge of the implementation of the Synchronized() method (i.e. the
method returns the original object, just having set the IsSynchronized
property to "true").

Of course, your original implementation appears to make that assumption
anyway (casting the returned object to Settings1), so maybe that's a
valid assumption to make in this context.

I still think I'd prefer an explicit suppression over twisting the code
like this just to satisfy the code analysis. :)

Pete
 |  Next  |  Last
Pages: 1 2
Prev: toolstrip - mouse event for disabled items?
Next: Calling Internal Method using Reflection