You know that any IDisposable object must be disposed using using. So, you have been using using to wrap WCF service’s ChannelFactory and Clients like this:
using(var client = new SomeClient()) {
.
.
.
}
Or, if you are doing it the hard and slow way (without really knowing why), then:
using(var factory = new ChannelFactory<ISomeService>()) {
var channel= factory.CreateChannel();
.
.
.
}
That’s what we have all learnt in school right? We have learnt it wrong!
When there’s a network related error or the connection is broken, or the call is timed out before Dispose is called by the using keyword, then it results in the following exception when the using keyword tries to dispose the channel:
failed: System.ServiceModel.CommunicationObjectFaultedException :
The communication object, System.ServiceModel.Channels.ServiceChannel,
cannot be used for communication because it is in the Faulted state.
Server stack trace:
at System.ServiceModel.Channels.CommunicationObject.Close(TimeSpan timeout)
Exception rethrown at [0]:
at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke(MessageData& msgData, Int32 type)
at System.ServiceModel.ICommunicationObject.Close(TimeSpan timeout)
at System.ServiceModel.ClientBase`1.System.ServiceModel.ICommunicationObject.Close(TimeSpan timeout)
at System.ServiceModel.ClientBase`1.Close()
at System.ServiceModel.ClientBase`1.System.IDisposable.Dispose()
There are various reasons for which the underlying connection can be at broken state before the using block is completed and the .Dispose() is called. Common problems like network connection dropping, IIS doing an app pool recycle at that moment, some proxy sitting between you and the service dropping the connection for various reasons and so on. The point is, it might seem like a corner case, but it’s a likely corner case. If you are building a highly available client, you need to treat this properly before you go-live.
So, do NOT use using on WCF Channel/Client/ChannelFactory. Instead you need to use an alternative. Here’s what you can do:
First create an extension method.
public static class WcfExtensions
{
public static void Using<T>(this T client, Action<T> work)
where T : ICommunicationObject
{
try
{
work(client);
client.Close();
}
catch (CommunicationException e)
{
client.Abort();
}
catch (TimeoutException e)
{
client.Abort();
}
catch (Exception e)
{
client.Abort();
throw;
}
}
}
Then use this instead of the using keyword:
new SomeClient().Using(channel => {
channel.Login(username, password);
});
Or if you are using ChannelFactory then:
new ChannelFactory<ISomeService>().Using(channel => {
channel.Login(username, password);
});
Enjoy!
This is weird. “Never throw an exception from a Dispose or Finalizer method”- this is what I always assumed as a general API design guidance. In fact Brad and Krzysztof emphasized this practice into their famous book. Don’t know why WCF has this exception.
Anyway, Thanks for the information 🙂
It’s hard to avoid exceptions in Dispose or Finalizer when you have to cleanup any I/O stuff or have to release some Win32 resource. I am sure this applies to any .net class which wraps file, tcp, http, win32 stuff. Trying to close them in Dispose/Finalizer can throw further exception any time and there’s nothing we can do about it.
In this case it doesn’t have to do with win32 stuff. This specific problem occurs with session based channels like TCP because the server has an open session. If the channel faults or the connection drops, the server session is left open because the channel will no longer allow messages to be sent. The exception could have been fixed by the WCF team, but they felt that users should know when a clean close / disconnect was not made. It’s one of the most questionable design descisions they made while building WCF.
You’re right about the dangers of wrapping a WCF client in a ‘using’ block, but there are a few other alternatives. I asked a question on StackOverflow about this issue a while back and got some good suggestions.
http://stackoverflow.com/questions/573872
Personally, I like to create my own partial class for the client and override the Dispose() method. This allows me to use the ‘using’ block as I normally would.
public partial class SomeWCFServiceClient : IDisposable
{
void IDisposable.Dispose()
{
if (this.State == CommunicationState.Faulted)
{
this.Abort();
}
else
{
this.Close();
}
}
}
Why not create a generic wrapper object to use with all the service classes? This would allow you to call multiple methods before disposing the client… That is what I did and it works perfectly.
Can give some code example?
Something in the lines of:
public class WcfClientWrapper : IDisposable where T : ICommunicationObject, IDisposable, new()
{
private T _client;
public WcfClientWrapper()
{
_client = new T();
}
public T Client
{
get { return _client; }
}
#region IDisposable Members
public void Dispose()
{
//…
}
#endregion
}
}
That way, just write:
using (new WcfClientWrapper(someclient))
{
//…
}
Just my two cents 🙂 Great blog, by the way 🙂
Good idea. But inside the using you have to do this:
using(var wrapper = new WcfClientWrapper(someClient))
{
ISomeClient client = wrapper.Client;
.
.
.
}
A lot more code than:
new SomeClient().Using(channel => {
.
.
.
});
Its a matter if preference, I believe. I prefer to make things simpler to read. Sometimes lambdas, in my opinion, just damage the readability of the code. Especially if the code within the using block is complex.
Don’t get me wrong. I like your implementation for the majority of the simple/medium complex cases. 🙂
Again, nice work. This topic is unknown to a lot of developers. At least until they bite them in the… Well… backside 🙂
I really like the using method and would still like to use it.
Would it not be possible to do something like below? Use the using statement and just handle the exceptions within it?
NotifyAllocationChangedResponse response = null;
using (NotifyAllocationChangedServiceClient testHostClient = new NotifyAllocationChangedServiceClient())
{
try
{
response = testHostClient.NotifyAllocationChanged(IntegrationSystemInformation.IntegrationSystemNotificationId);
}
catch (SoapException ex)
{
ErrorHandler.HandleError(“AM”, “AM.SendNotification”, ex);
}
catch (MessageSecurityException ex)
{
ErrorHandler.HandleError(“AM”, “AM.SendNotification”, ex);
}
catch (TimeoutException ex)
{
ErrorHandler.HandleError(“AM”, “AM.SendNotification”, ex);
}
catch (CommunicationException ex)
{
ErrorHandler.HandleError(“AM”, “AM.SendNotification”, ex);
}
catch (Exception ex)
{
ErrorHandler.HandleError(“AM”, “AM.SendNotification”, ex);
throw;
}
}
Imagine writing this hundred times for hundred different places in your code.
Then imagine making a change in the try…catch block in all those hundred areas.
High maintainability cost. Highly unreadable cost. Code does more housecleaning than the actual work.
Hi Omar, Thanks for the solution. I am using your solution in my service and it works fine except one situation. I don’t like but one of our service method (operation contract) using ref parameter. When I am trying to close the service client for that method, I am getting the error “Cannot use ref or out parameter ‘zzzz’ inside an anonymous method, lambda expression, or query expression”. I can remove the ref parameter and pass it as a part of the message but is there any other way to fix it?
oop way solution by me (Adapter/Wrapper)
public class ServiceClientSafeDisposingWrapper : IDisposable where T : ICommunicationObject
{
private readonly T _serviceClient;
///
/// Wrapped service client
///
public T ServiceClient
{
get { return _serviceClient; }
}
public ServiceClientSafeDisposingWrapper(T serviceClient)
{
if (serviceClient == null) throw new ArgumentNullException(“serviceClient”);
_serviceClient = serviceClient;
}
public void Dispose()
{
if (_serviceClient.State == CommunicationState.Faulted)
{
_serviceClient.Abort();
}
else if (_serviceClient.State != CommunicationState.Closed)
{
_serviceClient.Close();
}
}
}