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\$

migrated from stackoverflow.com Mar 11 '14 at 15:16

This question came from our site for professional and enthusiast programmers.

  • 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 Feb 5 '14 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 Mar 4 '14 at 15:34
  • \$\begingroup\$ MSMQ persists messages to disk \$\endgroup\$ – ChrisW Mar 11 '14 at 15:47
  • \$\begingroup\$ @ChrisW In this case, we don't need to persist the data. \$\endgroup\$ – B2K Mar 12 '14 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 Apr 13 '14 at 8:24
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\$
  • 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 Apr 14 '14 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\$ – Mathieu Guindon Apr 14 '14 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 Apr 14 '14 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 Apr 24 '14 at 14:25

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.