Monday, 24 November 2008

don't(don't(use using))

I was at DDD day on Saturday, and I was a little surprised to hear a recommendation "don't use using". Now, to be fair: I'm very familiar with the context of this quote - specifically, a handful of classes that do not provide a clean (non-throwing) implementation of IDisposable. But I still don't agree with the advice...

For example, it is fairly well known that WCF makes a bit of a mess in this area... and a complicated mess too. The core ClientBase class throws on Dispose() under a range of fault conditions - with the side effect that you can very easily end up accidentally losing your original exception to a using block.

The recommended workaround here was to not use "using", but to wrap the code manually - something like:

Proxy proxy = new Proxy();
try {
...
} finally {
// lots of code based on state of proxy
}
Now, we could move all the "lots of code" to a static method, but it still makes us do more work that we would like. With WCF, the scenario is further complicated because of the way that proxies work in terms of code-generation - it would be very messy to have to do any subclassing, since you would be subclassing an unknown subclass of ClientBase.

So in this scenario, I would greatly favor encapsulation... an in particular, C# 3.0 extension methods rescue us hugely. Consider we create a common base-class / interface to handle "something we want to dispose but which might explode on us":
public interface IDisposableWrapper<T> : IDisposable
{
T BaseObject {get;}
}
public class DisposableWrapper<T> : IDisposableWrapper<T> where T : class, IDisposable {
public T BaseObject {get;private set;}
public DisposableWrapper(T baseObject) { BaseObject = baseObject; }
protected virtual void OnDispose() {
BaseObject.Dispose();
}
public void Dispose() {
if (BaseObject != null) {
try {
OnDispose();
} catch { } // swallow...
}
BaseObject = null;
}
}
Now we can create some extension methods to wrap that for us...

    // core "just dispose it without barfing"
public static IDisposableWrapper<T> Wrap<T>(this T baseObject)
where T : class, IDisposable
{
if (baseObject is IDisposableWrapper<T>) return (IDisposableWrapper<T>)(object)baseObject;
return new DisposableWrapper<T>(baseObject);
}
But more - because extension methods support overloading, we can introduce a WCF-specific extension overload to support clean disposal of WCF clients:

public class ClientWrapper<TProxy, TService> : DisposableWrapper<TProxy>
where TProxy : ClientBase<TService>
where TService : class {
public ClientWrapper(TProxy proxy) : base(proxy) { }
protected override void OnDispose()
{
// lots of code per state of BaseObject
}
}
// specific handling for service-model
public static IDisposableWrapper<TProxy> Wrap<TProxy, TService>(
this TProxy proxy)
where TProxy : ClientBase<TService>
where TService : class
{
return new ClientWrapper<TProxy, TService>(proxy);
}
It might not be obvious, but the generic type-inference actually does all the heavy lifting here; we have a fully wrapped WCF client, regardless of the specific subclass of ClientBase - so we can use:


using (var client = new Proxy().Wrap()) {
client.BaseObject.SomeMethod();
}
Safe, robust, easy to call, and no code duplication. And extensible to other special cases too.

9 comments:

Jay Bazuzi said...

From Eric Lippert I learned that "pattern" means "workaround for language limitation", and this is a good example. We wish we didn't have to write code to solve this problem, but given that we must, here's a good way to solve it.

You could implement Wrap() as:

return baseObject as IDisposableWrapper< T > ?? new DisposableWrapper< T >(baseObject);

Marc Gravell said...

Eric is always good for a quote on any code topic ;-p

And yes - null-coalescing does seem to make it more legible, doesn't it? Good idea.

Ed said...

I thought the same Mark, but I'm not in a position to question, because I don't know enough about WCF.

In his defence the speaker did have ten items to get through in sixty min (not to mention bring his persona to the theatre, to keep our attention?)

I guess the issue here is that there's a code smell in not using a using.

Your snippet is pretty cool - but I must confess suppressing an exception in an empty general catch is a also a pretty unpleasant code smell?

I suppose you're snippet abstracts the 'issue' and as such is DRY so at least one can isolate and manage any weirdness. Rome, wasn't build in a day ;)

Marc Gravell said...

I spoke to the speaker subsequently, and he largely agreed with the issue - time constraints were against him, though.

SlashEne said...

You can Wrap ICommunicationObject instead of IDisposable because in WPF it's the type which use proxies created by the factory and by svcutil.
Moreover, I think a good thing to do is to close in another thread, because Close is very very slow, and we don't need anything else.

Sudheer said...

Thanks Marc.
If I am getting the proxy using factory.CreateChannel() (not using ServiceReference), how can I solve this issue?

Sudheer said...

Here is my solution

http://geekswithblogs.net/SudheersBlog/archive/2009/09/01/134430.aspx

Marc Gravell said...

That'll do it ;-p Interesting.

Anonymous said...

Sometimes I want to be able to customize the dispose-method. For example, when I want specific exceptions for WCF objects caught under specific conditions.

In the end I wrote an extension which gives me the possibility to make every object disposable, ICommunicationObjects too.


http://pastebin.com/VCPnCi0h