3
\$\begingroup\$

I am trying to optimize the usage of TopicClients in my app. The instance is originally covered by interface and passed by IoC, so the IoC will dispose it at the end of the program life cycle. This way I think I am saving some resources for the create/dispose of the client.Is such an implementation safe? Are there any pitfalls?

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Core.Model.Services;
using Microsoft.ServiceBus;
using Microsoft.ServiceBus.Messaging;
using Microsoft.WindowsAzure;

namespace Core.Persistence.Infrastructure.Implementation
{
    public class DefaultAzureServiceBusHelper : IAzureServiceBusHelper
    {
        private readonly string _defaultConnectionString;
        private readonly IValidationService _validationService;
        private object _padLock = new Object();

        private static ConcurrentDictionary<string, TopicClient> _topicClients = new ConcurrentDictionary<string, TopicClient>();
        private static ConcurrentDictionary<string, SubscriptionClient> _subscriptionClients = new ConcurrentDictionary<string, SubscriptionClient>();

        public NamespaceManager NamespaceManager { get; set; }
        public DefaultAzureServiceBusHelper(IValidationService validationService)
        {
            _validationService = validationService;
            _defaultConnectionString = CloudConfigurationManager.GetSetting("Microsoft.ServiceBus.ConnectionString");
        }


        public NamespaceManager GetNamespaceManager(string connectionString = null)
        {
            lock(_padLock)
            {
                if(this.NamespaceManager == null)
                {
                    this.NamespaceManager = NamespaceManager.CreateFromConnectionString(connectionString ?? _defaultConnectionString);
                }
            }
            return this.NamespaceManager;
        }

        public TopicDescription CreateTopic(string path)
        {
            _validationService.StringIsNullOrEmpty(path, "path");

            try
            {
                if(!this.NamespaceManager.TopicExists(path))
                {
                    return this.NamespaceManager.CreateTopic(path);
                }
            }
            catch(MessagingEntityAlreadyExistsException)
            {
                // intended because the topic is simultaneously created by all isntances
            }
        }

        public TopicDescription CreateTopic(TopicDescription description)
        {
            _validationService.Null(description, "description");
            try
            {
                if(!this.NamespaceManager.TopicExists(description.Path))
                {
                    return this.NamespaceManager.CreateTopic(description);
                }
            }
            catch(MessagingEntityAlreadyExistsException)
            {
                // intended because the topic is simultaneously created by all isntances
            }
        }


        public SubscriptionDescription CreateSubscription(string path, string name)
        {
            _validationService.StringIsNullOrEmpty(path, "path");
            _validationService.StringIsNullOrEmpty(path, "name");
            try
            {
                if(!this.NamespaceManager.SubscriptionExists(path, name))
                {
                    return this.NamespaceManager.CreateSubscription(path, name);
                }
            }
            catch(MessagingEntityAlreadyExistsException)
            {
                // intended because the topic is simultaneously created by all isntances
            }
        }

        public SubscriptionDescription CreateSubscription(SubscriptionDescription description)
        {
            _validationService.Null(description, "description");
            try
            {
                if(!this.NamespaceManager.SubscriptionExists(description.Path))
                {
                    return this.NamespaceManager.CreateSubscription(description);
                }
            }
            catch(MessagingEntityAlreadyExistsException)
            {
                // intended because the topic is simultaneously created by all isntances
            }
        }


        public TopicClient GetTopicClient(string topic)
        {
            _validationService.StringIsNullOrEmpty(topic, "topic");

            if(_topicClients.ContainsKey(topic) && !_topicClients[topic].IsClosed)
            {
                return _topicClients[topic];
            }
            else
            {
                var newTopicClient = TopicClient.CreateFromConnectionString(this._defaultConnectionString, topic);
                _topicClients.AddOrUpdate(topic, newTopicClient, (ik, existingV) => newTopicClient);
            }
        }

        public SubscriptionClient GetSubsriptionClient(string topic, string name)
        {
            _validationService.StringIsNullOrEmpty(topic, "topic");
            _validationService.StringIsNullOrEmpty(name, "name");

            if(_subscriptionClients.ContainsKey(topic) && !_subscriptionClients[topic].IsClosed)
            {
                return _subscriptionClients[topic];
            }
            else
            {
                var newSubscriptionClient = SubscriptionClient.CreateFromConnectionString(this._defaultConnectionString, topic, name);
                _subscriptionClients.AddOrUpdate(topic, newSubscriptionClient, (ik, existingV) => newSubscriptionClient);
            }
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

One of the goals of IoC is to write testable code by decoupling dependencies.

Constructor Injection is the ideal way to do this, and with proper DI you inject abstractions rather than concrete implementation - this is the key to testable code, because then you can swap all the dependencies for mocks and test only the relevant code.

private readonly string _defaultConnectionString;
private readonly IValidationService _validationService;

public DefaultAzureServiceBusHelper(IValidationService validationService)
{
    _validationService = validationService;
    _defaultConnectionString = CloudConfigurationManager.GetSetting("Microsoft.ServiceBus.ConnectionString");
}

I see 2 dependencies here, but only 1 is getting constructor-injected, and the other seems to be a static class that can be accessed anywhere by anyone at any time.

If CloudConfigurationManager really needs to be accessed anywhere by anyone at any time, it's part of a cross-cutting concern. There's a DI pattern to address this, called Ambient Context, with its pros and cons - read up on it in Mark Seemann's Dependency Injection in .NET, an excellent read.

I doubt CloudConfigurationManager really is part of a cross-cutting concern though. If it's a static class that's part of a framework, you should wrap it with an interface and constructor-inject it as well.

public interface IConfigurationManager
{ 
    string GetSetting(string key);
}

public class CloudConfigurationManagerWrapper : IConfigurationManager
{
    public string GetSetting(string key)
    {
        return CloudConfigurationManager.GetSetting(key);
    }
}

And then your constructor's signature can look like this:

public DefaultAzureServiceBusHelper(IValidationService validationService, IConfigurationManager config)

...but, wait a minute, does DefaultAzureServiceBusHelper really depend on an IConfigurationManager? All we're really after, is the default connection string! Can't we have something more like this?

public DefaultAzureServiceBusHelper(IValidationService validationService, string defaultConnectionString)

Of course we can! And we should! If no class actually depends on an IConfigurationManager, then the CloudConfigurationManager should be used at the composition root, to consructor-inject the defaultConnectionString and other configuration settings needed throughout the application.


This is a very confusing API:

public NamespaceManager NamespaceManager { get; set; }

public NamespaceManager GetNamespaceManager(string connectionString = null)
{
    lock(_padLock)
    {
        if(this.NamespaceManager == null)
        {
            this.NamespaceManager = NamespaceManager.CreateFromConnectionString(connectionString ?? _defaultConnectionString);
        }
    }
    return this.NamespaceManager;
}

NamespaceManager has a public setter, so anyone anywhere can set it to anything. And it has a public getter, so anyone anywhere can retrieve it without ever calling GetNamespaceManager - get rid of the auto-property, it's nothing more than a public field with a property's makeup at that point.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. I will implement the changes. Further more I think to move to strong-typed config. Here in 1,2 articles I have found how to do it properly. I have read M. Seeman`s good book and got the validation cross-cutting but have overlooked the configuration problem. I will post the result when I am ready. \$\endgroup\$ – Ognyan Dimitrov Jun 25 '15 at 7:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.