Throwing exception in the singleton pattern in C#

Question

I'm implementing an HTTP client shell that's going to be used for the lifetime of my application in C#. It needs to be initialized before the instance can be accessed. It looks something like this:

public static class AssetSyncServiceClient {
 private static object _lock = new object();
 private static bool _initialized = false;
 private static HttpClient _client = null;
 /// <summary>
 /// Indicates whether the client has been initialized.
 /// </summary>
 /// <returns>Whether the client is initialized.</returns>
 public static bool IsInitialized() {
 lock (_lock) {
 return _initialized;
 }
 }
 /// <summary>
 /// Creates and configures HTTP client.
 /// </summary>
 public static void Initialize(string baseUrl, bool onlyRequestJson = true) {
 lock (_lock) {
 if (_initialized) { throw new Exception("Client has already been initialized."); }
 _client = new HttpClient();
 _client.BaseAddress = new Uri(baseUrl);
 if (onlyRequestJson) {
 _client.DefaultRequestHeaders.Accept.Clear();
 _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
 }
 _initialized = true;
 }
 }
 /// <summary>
 /// Closes / disposes of the HTTP client.
 /// </summary>
 public static void Close() {
 lock (_lock) {
 if (_initialized && _client != null) {
 _client.Dispose();
 }
 }
 }
 /// <summary>
 /// Gets client instance.
 /// </summary>
 public static HttpClient Instance {
 get {
 lock (_lock) {
 if (!_initialized) { throw new Exception("HTTP client has not been initialized."); }
 return _client;
 }
 }
 }
}

The trouble is, this throws up a warning in Visual Studio because of style rule CA1065 - Microsoft say you shouldn't throw an exception in a property. I just don't agree with this - it seems fine to me. If you try to access .Instance before it's initialized you get an exception. It's clearly a property whose code could throw an exception, and not a "dumb field". I could turn Instance into a GetInstance method but I just prefer the look of accessing it from calling code as a property.

So does it make sense to just suppress the warning here, or am I designing this badly? Is there a better way? Should I bite the bullet and make Instance a method?

UPDATE:
It looks like I probably want something other than a singleton because singletons are hard to test (although actually MS Fakes can mock static methods so maybe it's OK), so what would be a better design pattern for this functionality that's unit testable?

• BCdotWEB: Somewhat related: codereview.stackexchange.com/questions/69950/…

Answer 1

• The initialize dance is not nice. Requires the caller to keep track of if it has been called etc. Pit of failure.
• The IsInitialized() is not very useful as it is now as there no way to lock over check and initialize. There will always be a risk that another thread calls Initialize between the check IsInitialized and the call to Initialize.
• I have not used HttpClient much but my gut feeling is that the locking is not needed.
• A singleton will make it hard to mock in tests. What about writing it like this:

public class AssetSyncServiceClientSettings
{
 public string BaseUrl { get; set; }
 public bool JsonOnly { get; set; }
}
public interface IAssetSyncServiceClient : IDisposable
{
 // Picked a sample method randomly, you can expose an api that makes sense in your app/lib
 Task<string> GetStringAsync(string requestUri);
}
public sealed class AssetSyncServiceClient : IAssetSyncServiceClient
{
 private readonly HttpClient _client;
 private bool _disposed;
 public AssetSyncServiceClient(AssetSyncServiceClientSettings settings)
 {
 _client = new HttpClient();
 _client.BaseAddress = new Uri(settings.BaseUrl);
 if (settings.JsonOnly)
 {
 _client.DefaultRequestHeaders.Accept.Clear();
 _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
 }
 }
 public Task<string> GetStringAsync(string requestUri)
 {
 VerifyNotDisposed();
 return _client.GetStringAsync(requestUri);
 }
 public void Dispose()
 {
 if (_disposed)
 {
 return;
 }
 _disposed = true;
 _client.Dispose();
 }
 private void VerifyNotDisposed()
 {
 if (_disposed)
 {
 throw new ObjectDisposedException(this.GetType().FullName);
 }
 }
}

• Doing it like this avoids initialization. If you need it lazy for some reason the service could handle it internally.
• An interface probably makes sense for this so you can mock it in tests.
• Use the Dependency Injection container to make the service a singleton instance.
• Pass in the service interface in to constructors where it is needed. It makes the dependency explicit and makes it easy to mock in tests.
• The settings object can perhaps contain urls so that the service can expose methods like public async Task<Stuff> GetStuffAsync()

• Jez: Good answer. One point I don't quite understand is "Pass in the service interface in to constructors where it is needed." - is that just a general principle, or are you referring to somewhere you have done this in your example code?
• Johan Larsson: It is a general principle, constructor injection, not shown in the sample code. Or in a way it is, we pass in AssetSyncServiceClientSettings in the constructor of the client. Pass in IAssetSyncServiceClient the same way where the service is used.
• Jez: One way I'm thinking of making this a singleton (without the complexity of a full Dependency Injection framework) is by using a "singleton factory"; a factory that creates/gets only one instance of an IAssetSyncServiceClient. Does that sound like a decent compromise?
• Johan Larsson: Could be but you want it written in a way so that it can return a mock in tests I think. IoC frameworks are not hard, I can walk you through it later when I get home. Imo no app is too small for IoC. Ping me in chat in a couple of hours.
• Jez: Which chatroom?