Tuesday, August 2, 2011

WCF Client Disposal Broken!

While running a recently-developed application I wrote through the Visual Studio 2010 code analyzer, I was told that I wasn't calling Dispose on an object which implemented IDispose.  Chagrined, I took a look and found that WCF client objects, derived from ClientBase, do indeed derive from IDispose.

Oddly, the autocomplete didn't allow me to test this, by attempting to call the Dispose method directly off the client object.  It turns out this is a known oddity; the Dispose method in ClientBase is private.  The reason that "using" would work is that using will effectively cast the client object back to IDisposable, where Dispose is once again callable:


But wait, that's not really why I'm posting.  Apparently the ClientBase's Dispose method calls Close, which can throw if there are errors performing the close.  Apparently, throwing exceptions out of Dispose is not recommended by Microsoft's own "Framework Design Guidelines":


What this means for a WCF client is that if you use the "using" approach for cleanup, any errors that occur during the implicit Dispose call at the end of the using block will mask any exceptions which occurred in the using block.

There is an extraordinarily ungainly approach recommended by Microsoft:


in which you create a try block with a call to Close() and explicitly provide catch blocks for each of CommunicationException, TimeoutException, and Exception; and in each of those blocks, you call Abort; and in the Exception catch block, you re-throw the original error after calling Abort().  No way I'm doing that.

Suggested elsewhere was what seemed a cleaner variation to me:

bool isSuccessful = false;
MyClient myClient = new MyClient();
try
{
    // use the client object here
    myClient.Close();
    isSuccessful = true;
}
finally
{
    if (!isSuccessful)
        myClient.Abort();
}

What a pain!  Why would the WCF developers do this to us?  Some notes on the internal discussion are available here:



and I think the pertinent part of that second link (well worth the read in its entirety!) is:
After much debate, IDisposable was left on ServiceHost and ClientBase, the theory being that for many users, it’s ok if Dispose throws, they still prefer the convenience of using(), and the marker that it should be eagerly cleaned up.  You can argue (and some of us did) that we should have removed it from those two classes as well, but for good or for ill we have landed where we have. It’s an area where you will never get full agreement, so we need to espouse best practices in our SDK samples, which is the try{Close}/catch{Abort} paradigm.
There were two decisions to make: should the client object inherit IDisposable; and if yes, should Dispose call Close or Abort?  All three outcomes (not IDisposable, Dispose calls Close, and Dispose calls Abort) are ugly in one way or another.  So how come other self-cleaning-up objects don't have this ugliness?

I guess I'm going to have to put up with warnings from code analyzer about missing calls to Dispose.  [sniff]

No comments:

Post a Comment