0
\$\begingroup\$

I am creating a web service that uses a component I've licensed that is restricted to two concurrent instances at any one time. Creating a new instance of the component takes a long time (seconds) so I want to maintain a pool of objects for re-use. Here's my first (working) attempt, but it seems fairly inelegant, and also likely that I've made some mistakes (although it hasn't failed in my own tests):

public class MyWebService : System.Web.Services.WebService
{
    static RestrictedClass rc1, rc2;

    [WebMethod]
    public byte[] MyMethod(byte[] Stream1, byte[] Stream2)
    {
        RestrictedClass rc = null;
        Mutex m1 = null, m2 = null;
        if (rc1 == null)
        {
            m1 = new Mutex(true, "rc1");
            m1.WaitOne();
            rc1 = new RestrictedClass();
            rc = rc1;
        }
        else if (rc2 == null)
        {
            m2 = new Mutex(true, "rc2");
            m2.WaitOne();
            rc2 = new RestrictedClass();
            rc = rc2;
        }
        else
        {
            bool m1Granted, m2Granted;
            m1 = new Mutex(true, "rc1", out m1Granted);
            m2 = new Mutex(true, "rc2", out m2Granted);
            int idx;
            if (m1Granted)
            {
                idx = 0;
            }
            else if (m2Granted)
            {
                idx = 1;
            }
            else
            {
                idx = Mutex.WaitAny(new Mutex[] { m1, m2 });
            }
            if (idx == 0)
            {
                rc = rc1;
                m2.Close();
            }
            else if (idx == 1)
            {
                rc = rc2;
                m1.Close();
            }
        }

        Result = rc.DoStuff();

        if (m1 != null && rc == rc1)
            m1.ReleaseMutex();
        if (m2 != null && rc == rc2)
            m2.ReleaseMutex();
        return Result.ToArray();
    }
}

Please pick it apart and suggest any improvements you like.

\$\endgroup\$
3
  • \$\begingroup\$ Code Review requires real code from a project. As a rule, placeholder identifiers are not acceptable. Please clarify your question by putting in real (or at least realistic) code. \$\endgroup\$ – 200_success Oct 17 '17 at 23:03
  • \$\begingroup\$ This code is taken almost exactly as is from my project. Yes, I changed the name of the web service class, the method, and the licensed component, but that's about it. It should not affect anyone's ability to review the code. \$\endgroup\$ – matthewk Oct 17 '17 at 23:05
  • \$\begingroup\$ Even when obfuscating the code, you could have picked names that sill make sense, thus -1. MyMethod could be GetComponentFromPool or anything else that is at least meaningful. MyWebService could be ComponentPoolService. And all these abbreviations. I find your sample is almost incomprehensible. If your real code looks like this then I would't want to maintain it. \$\endgroup\$ – t3chb0t Nov 5 '17 at 7:16
2
\$\begingroup\$

I have plenty of things to say here..

  1. You should apply separation of concerns by isolating the pooling behavior from the logic of the webmethod itself.
  2. you might want to create both instances at startup
  3. blocking inside the web methods will seriously limit throughput, consider using async methods
  4. What happens if there is an exception and you don't release the mutex?
  5. The pooling can be implemented much simpler using higher level concurrency features, for example a BlockingCollection:

public class ObjectPool<T> 
{
    private readonly BlockingCollection<Lazy<T>> _pool;

    public ObjectPool(int size, Func<T> constructor)
    {
        var resources = Enumerable.Repeat(new Lazy<T>(constructor), size);
        var stack = new ConcurrentStack<Lazy<T>>(resources);
        _pool = new BlockingCollection<Lazy<T>>(stack);
    }

    public IDisposable Aquire(out T resource)
    {
        Lazy<T> lazy = _pool.Take();
        resource = lazy.Value;
        return new Disposable(() => _pool.Add(lazy));
    }

    public class Disposable : IDisposable
    {
        private readonly Action _dispose;

        public Disposable(Action dispose)
        {
            _dispose = dispose;
        }

        public void Dispose()
        {
            _dispose.Invoke();
        }
    }
}

The call to Aquire will block until an item is available in the pool, it's very important to call Dispose on the returned object, this will release the resouce back into the pool. You can use try/finally or a using statement:

    private static readonly ObjectPool<Random> pool 
        = new ObjectPool<Random>(2, () => new Random());

    static int CalculateRandomNumber()
    {
        using (pool.Aquire(out var rnd))
        {
            return rnd.Next();
        }
    }

Notice how a Stack is used in combination with Lazy. This ensures that you will never create more instances than are in use at the same time.

\$\endgroup\$

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.