Best, thread safe way to access a field once i.e. for disposal

Question

Basically I want to make sure a field in a class (in this case the _changedPoller) is Disposed as early as possible when not needed anymore. I call StopChangedPolling in the class' Dispose method and when certain events occur. What is the best, thread safe way to dispose a field only once? Is the following code ok?

private void StopChangedPolling()
{
    IDisposable changedPoller = Interlocked.CompareExchange(ref _changedPoller, null, _changedPoller);
    if (changedPoller != null)
        changedPoller.Dispose();
}

The second time this is called, _changedPoller is null and no exception is thrown, even though the documentation states that an ArgumentNullException is thrown if the address of location1 is a null pointer.

In this case you can use `Interlocked.Exchange` instead of `CompareExchange`. — Kevin Gosse, Jun 18 '15 at 14:28

score 2 · Accepted Answer · answered Jun 18 '15 at 14:38

2

You are not passing a null pointer, you are passing a reference to a null reference. I don't think you can provoke the exception

The address of location1 is a null pointer.

in C# because you can't create a null managed pointer in C#.

This is thread-safe but will fail to dispose in case of a thread abort. If that is not a concern for you then this is fine.

You could consider to simplify this code by using a lock but I actually think this is simpler than a lock.

You also can use Interlocked.Exchange.

Actually, there is one concern: You are reading _changedPoller in an unsynchronized way. I'm not sure what guarantees ECMA and the .NET CLR make around this. I would have to read the spec. volatile would fix that. Interlocked.Exchange would fix it too if there are no other such read usages.

answered Jun 18 '15 at 14:38

usr

168,620
35
240
369

`You are reading _changedPoller in an unsynchronized way` Aren't the Interlocked method supposed to use a memory barrier? – Kevin Gosse Jun 18 '15 at 14:43
2

It does, but one of the arguments is simply `_changedPoller`. That's a normal read. – usr Jun 18 '15 at 14:43
Oh, nice catch. Even with volatile, there's a racing condition here. `Interlocked.Exchange` should definitely be used instead – Kevin Gosse Jun 18 '15 at 14:46
This stuff is so hard and fragile. I overlooked this at first. My recommendation to use a lock seems even wiser now. – usr Jun 18 '15 at 14:47

Best, thread safe way to access a field once i.e. for disposal

1 Answers1