3
\$\begingroup\$

I'm in the middle of a project that was built around Microsoft's Message Queue service. During development all of our machines are on a domain and we were able to create a public queue accessible from the server and client.

When moving this to our test server, I discovered that both the test server and production servers are running in workgroup mode, no public queues available.

I'm experimenting with creating a webservice broker backed by a MemoryCache. Here is what I've got so far. Each key is posted into a FIFO queue due to the needs of the project.

Has anyone else done something similar? What about concurrency? Any other gotchas I should be concerned about?

UPDATED: Added suggested changes, and handled differences resulting from use of ConcurrentQueue and Lazy instantiation.

    [WebMethod]
    public bool Push(string key, string value)
    {
        if (String.IsNullOrWhiteSpace(key) || String.IsNullOrWhiteSpace(value)) return false;

        // input validation removed for brevity

        var queue = GetQueue(key);
        queue.Enqueue(value);                
        return true;
    }

    [WebMethod]
    public string Pop(string key)
    {
        var queue = GetQueue(key);
        string result = "";
        if (queue.TryDequeue(out result))
        {
            return result;
        }
        return null;                
    }

    [WebMethod]
    public List<string> RemoveAll(string key)
    {
        var queue = GetQueue(key);
        var list = queue.ToList();
        queue.Clear();
        return list;
    }

    private ConcurrentQueue<string> GetQueue(string key)
    {
        var cache = MemoryCache.Default;
        var queue = (Lazy<ConcurrentQueue<string>>) (cache.AddOrGetExisting(key.ToLowerInvariant(),new Lazy<ConcurrentQueue<string>>() , new CacheItemPolicy
        {
            AbsoluteExpiration = ObjectCache.InfiniteAbsoluteExpiration,
            SlidingExpiration = TimeSpan.FromMinutes(5)
        }) ?? cache[key.ToLowerInvariant()]);
        return queue.Value;
    }

I also added an extension for the ConcurrentQueue to clear it.

public static class ConcurrentQueueExtensions
{
    public static void Clear<T>(this ConcurrentQueue<T> queue)
    {
        T item;
        while (queue.TryDequeue(out item))
        {
            // do nothing
        }
    }
}
\$\endgroup\$
9
  • 2
    \$\begingroup\$ Just use private queues, they are still accessable remotely. See technet.microsoft.com/en-us/library/cc778392(v=ws.10).aspx. Public queues are a pain anyways, so this would be better overall. \$\endgroup\$
    – Travis
    Commented Feb 5, 2014 at 16:57
  • \$\begingroup\$ I went ahead and created my memory cache web service. It has been a great addition to our applications, providing an extremely lightweight messaging service without any of the fuss. \$\endgroup\$
    – B2K
    Commented Mar 4, 2014 at 15:34
  • \$\begingroup\$ MSMQ persists messages to disk \$\endgroup\$
    – ChrisW
    Commented Mar 11, 2014 at 15:47
  • \$\begingroup\$ @ChrisW In this case, we don't need to persist the data. \$\endgroup\$
    – B2K
    Commented Mar 12, 2014 at 21:42
  • 1
    \$\begingroup\$ What about concurrency in multi threaded scenarios. Is it possible that Pop() and Push() is being called concurrently, because then the GetQueue() might turn out to be a problem. The time from looking up the queue in the memory cache and until you try to add it, might turn out to be a race-condition. \$\endgroup\$
    – ahybertz
    Commented Apr 13, 2014 at 8:24

1 Answer 1

3
\$\begingroup\$

What you have here is, IMHO, excellent.

Couple nitpicks:

  • I don't get why RemoveAll is returning the original content instead of void - also I'm expecting an exception to be thrown in GetQueue if the specified key doesn't exist.
  • This part (in the Pop(string) method):

    var queue = GetQueue(key);
    string result = "";
    

    Would look more consistent if written like this:

    var queue = GetQueue(key);
    var result = string.Empty;
    

    Or, since it's used as an out parameter value on the next line (and therefore is compiler-enforced to be initialized after the call to TryDequeue(out string)), simply like this:

    var queue = GetQueue(key);
    string result;
    

As for the Clear extension method, there's a potential tiny little issue here - see this MSDN thread:

ConcurrentQueue<T>'s implementation is based on a particular lock-free algorithm that doesn't permit atomically clearing of the collection, hence why no Clear method is exposed. As such, you have two primary options.

The first is simply to create a new ConcurrentQueue<T> instead of clearing the original, and then swap in the new for the original.

The second is to continually remove from the collection until it's empty, e.g. T ignored; while(cq.TryDequeue(out ignored));. Which you choose is largely dependent on the needs of your application.

The former will be faster, but you also run the risk of losing some items you didn't intend to lose (then again, if the whole point is to empty the collection, that might not matter), and it only works if you have access to all of the variables holding references to the queue.

The latter is effective, but it runs the risk of running indefinitely if other threads are concurrently appending; it's also more expensive that simply allocating a new queue.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Perhaps RemoveAll would be clearer if it were PopAll instead. As this is now in production, I'll leave the name alone. I'm not throwing an exception because it's not needed in my case, returning null has the same net effect. I might also return an empty string. Also due to the implementation, this will always either add or get a new queue, if the key isn't null or empty. I'll update with String.Empty. Bad habits on my part. \$\endgroup\$
    – B2K
    Commented Apr 14, 2014 at 19:06
  • \$\begingroup\$ @B2K hey! nice to hear from you! (never sure with migrated questions..) - sorry this post took forever to get an answer, I really hope future Code Review experiences will be better! (I'm out of votes right now, but I'll come back tonight to upvote your question here ;) \$\endgroup\$ Commented Apr 14, 2014 at 19:13
  • 1
    \$\begingroup\$ Regarding the Clear extension, assigning a new queue would not update the entry in the cache. The original reference in the cache would remain unchanged. There is a possibility that new items could get removed before they have been read, in my case, it's not critical, as the incremental results are superseded by the completion of an asynchronous callback. (It's the quote page for autoquoter.com -- zipcode=60610) If retrieval of all results is required, pop could always be used. \$\endgroup\$
    – B2K
    Commented Apr 14, 2014 at 19:16
  • \$\begingroup\$ @Mats I've updated the question with a fix. AddOrGetExisting returns a null if the item is added. One more comment on the Clear extension. Since the cached queue is set to timeout after 5 minutes, there was always a possibility that items posted to the queue would disappear before they were read. \$\endgroup\$
    – B2K
    Commented Apr 24, 2014 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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