Is it good idea to throw an exception in .NET C# constructor?

· Read in about 7 min · (1301 words) ·

I got reminded recently about two cases why it may be actually a bad idea to do a complex initialization in a class constructor in C#.

All the code samples below are written just for the purpose of this post but are based on real code I either written or came across at some point.

Case 1 - instance resolved via an IoC container

Recently I was facing an issue that application started but remained non-functional.

I quickly found following stack trace in the log file.

Unity.Exceptions.ResolutionFailedException: Resolution of the dependency failed, type = 'IMyDataReader', name = '(none)'.
Exception occurred while: Calling constructor MyDataReader().
Exception is: AggregateException - One or more errors occurred.
-----------------------------------------------
At the time of the exception, the container was:
  Resolving MyDataReader,(none) (mapped from IMyDataReader, (none))
  Calling constructor MyDataReader()
 ---> System.AggregateException: One or more errors occurred. ---> System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.WebException: Unable to connect to the remote server ---> System.Net.Sockets.SocketException: No connection could be made because the target machine actively refused it 127.0.0.1:1234
   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult)
   at System.Net.ServicePoint.ConnectSocketInternal(Boolean connectFailure, Socket s4, Socket s6, Socket& socket, IPAddress& address, ConnectSocketState state, IAsyncResult asyncResult, Exception& exception)
   --- End of inner exception stack trace ---
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Net.Http.HttpClientHandler.GetResponseCallback(IAsyncResult ar)
   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at MyDataReader..ctor() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 117
   at lambda_method(Closure , IBuilderContext )
   at Unity.ObjectBuilder.BuildPlan.DynamicMethod.DynamicBuildPlanGenerationContext.<>c__DisplayClass16_0.<GetBuildMethod>b__0(IBuilderContext context) in C:\projects\unity\Container\src\ObjectBuilder\BuildPlan\DynamicMethod\DynamicBuildPlanGenerationContext.cs:line 134
   at Unity.ObjectBuilder.Strategies.BuildPlanStrategy.PreBuildUp(IBuilderContext context) in C:\projects\unity\Container\src\ObjectBuilder\Strategies\BuildPlanStrategy.cs:line 36
   at Unity.Container.StrategyChain.BuildUp(IBuilderContext builderContext) in C:\projects\unity\Container\src\Container\StrategyChain.cs:line 54
   at Unity.Policy.BuildPlanPolicyExtensions.ExecuteBuildUp(IBuildPlanPolicy policy, IBuilderContext context) in C:\projects\unity\Abstractions\src\Policy\IBuildPlanPolicy.cs:line 36
   at Unity.UnityContainer.BuildUp(Type typeToBuild, Object existing, String name, ResolverOverride[] resolverOverrides) in C:\projects\unity\Container\src\UnityContainer.cs:line 203
   --- End of inner exception stack trace ---
   at Unity.UnityContainer.BuildUp(Type typeToBuild, Object existing, String name, ResolverOverride[] resolverOverrides) in C:\projects\unity\Container\src\UnityContainer.cs:line 215
   at Unity.UnityContainer.Resolve(Type type, String name, ResolverOverride[] resolverOverrides) in C:\projects\unity\Container\src\UnityContainer.cs:line 163
   at Unity.UnityContainerExtensions.Resolve[T](IUnityContainer container, ResolverOverride[] overrides) in C:\projects\unity\Abstractions\src\Utility\UnityContainerExtensions.cs:line 469
   at T.TT() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 97

As you can see - the log is pretty cryptic. The source in the end led to following piece of code:

public class MyDataReader : IMyDataReader
{
    string _name;
    public MyDataReader()
    {
        var http = new HttpClient();
        _name = http.GetStringAsync("http://localhost:1234").Result;
    }

    public string Name { get { return _name; } }
}

The above idea is pretty clear - load the data from local data service and since those cannot change just cache them.

The assumption was that the service is always running (this was, due to the design, actually a good one - my system just didn’t have expected setup yet).

So what is wrong here?

Well - in this particular case the bad part actually was that the class was registered in Microsoft Unity IoC container.

As result upon an instance resolution which is the action running the constructor, the actual place which throws the exception gets quite hidden and non-obvious since container doesn’t know why the class couldn’t be constructed.

Ultimately the support doesn’t have basically any chances to find out the issue and fix it (which may be just starting the missing service).

Solution

In this particular case pretty simple - leave the class initialize and lazy load the actual data later like this:

public class MyDataReader : IMyDataReader
{
    Lazy<string> _name;
    public MyDataReader()
    {
        _name = new Lazy<string>(() =>
        {
            var http = new HttpClient();
            return http.GetStringAsync("http://localhost:1234").Result;
        });
    }

    public string Name { get { return _name.Value; } }
}

Which leads to much more readable log - and more importantly - the beginning of the message points quickly to actual issue:

System.AggregateException: One or more errors occurred. ---> System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.WebException: Unable to connect to the remote server ---> System.Net.Sockets.SocketException: No connection could be made because the target machine actively refused it 127.0.0.1:1234
   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult)
   at System.Net.ServicePoint.ConnectSocketInternal(Boolean connectFailure, Socket s4, Socket s6, Socket& socket, IPAddress& address, ConnectSocketState state, IAsyncResult asyncResult, Exception& exception)
   --- End of inner exception stack trace ---
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Net.Http.HttpClientHandler.GetResponseCallback(IAsyncResult ar)
   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at MyDataReader.<>c.<.ctor>b__1_0() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 119
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.LazyInitValue()
   at MyDataReader.get_Name() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 123
   at T.TT() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 97
---> (Inner Exception #0) System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.WebException: Unable to connect to the remote server ---> System.Net.Sockets.SocketException: No connection could be made because the target machine actively refused it 127.0.0.1:1234
   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult)
   at System.Net.ServicePoint.ConnectSocketInternal(Boolean connectFailure, Socket s4, Socket s6, Socket& socket, IPAddress& address, ConnectSocketState state, IAsyncResult asyncResult, Exception& exception)
   --- End of inner exception stack trace ---
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Net.Http.HttpClientHandler.GetResponseCallback(IAsyncResult ar)
   --- End of inner exception stack trace ---<---/

Case 2 - working with unmanaged resources

There is one more interesting scenario to be considered - initializing unmanaged/disposable resources within the class constructor.

public void Main()
{
    try
    {
        using (var r = new FReader())
        {

        }
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex);
    }

    try
    {
        using (var r = new FReader())
        {

        }
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex);
    }
}

public class FReader : IDisposable
{
    private Stream _input;

    public FReader()
    {
        _input = File.Open(@"C:\windows-version.txt", FileMode.Open, FileAccess.Read, FileShare.None);

        var content = _input.ReadByte();

        switch (content)
        {
            case 'a':
            case 'b':
                break;
            default:
                throw new InvalidOperationException("Unexpected input");
        }
    }

    public void Dispose()
    {
        _input.Dispose();
    }
}

The class seems to be pretty robust, right? It disposes the file properly so behaves nicely.

Nothing wrong, at least for the first look.

But let’s look into exceptions being thrown.

First instantiation

System.InvalidOperationException: Unexpected input
   at X.FReader..ctor() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 176
   at X.Main() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 135

Second instantiation

System.IO.IOException: The process cannot access the file 'C:\windows-version.txt' because it is being used by another process.
   at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
   at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
   at System.IO.File.Open(String path, FileMode mode, FileAccess access, FileShare share)
   at X.FReader..ctor() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 164
   at X.Main() in C:\Users\vladi\Documents\code\UnityInitializer\UnityInitializer.Tests\RootContainerSetupTest.cs:line 147

As you can see for the second case - the file is still locked! So it didn’t dispose.

So what is wrong here?

In the constructor there is an assumption that the file must have a certain structure.

But what if the file is invalid?

It will throw an exception in constructor. Hmm - still good?

It depends!

As the exception is thrown - the above level assignment within the using statement will never happen here.

As result - the Dispose() method will never be called.

Which means that the resource (in our case file) remains active. The above is not actually 100% correct all the time.

Depending on the resource type (for example some unmanaged resource with finalizer) - you may observe that the resource got at some point cleaned up during garbage collection.

Solution

  • You may consider something like this:

    public class FReader : IDisposable
    {
       private Stream _input;
    
       public FReader()
       {
           _input = File.Open(@"C:\windows-version.txt", FileMode.Open, FileAccess.Read, FileShare.None);
    
           try
           {
               var content = _input.ReadByte();
    
               switch (content)
               {
                   case 'a':
                   case 'b':
                       break;
                   default:
                       throw new InvalidOperationException("Unexpected input");
               }
           }
           catch (Exception ex)
           {
               _input.Dispose();
               _input = null;
           }
       }
    
       public void Dispose()
       {
           _input.Dispose();
       }
    }

    But you will probably very quickly realize that with slightly more complex initialization this will become nightmare. Also - the object will not be actually properly constructed.

  • Much easier will actually be avoiding such stuff in constructor and leave it for later - similar to the first case.

Conclusion

Always keep in mind what the impact of complex logic within constructor might be.

You may shoot yourself and observe really weird non-deterministic issues or repeating support cases because of unreadable log files.

For me personally - avoiding the complex logic during the object construction is the preferred way of coding.