0

I'm going through following method which is sending messages over Http.

        private static void HTTPProcessQueue()
        {
            while (true)
            {
                try
                {
                    Thread.Sleep(10000);
                    Utils.LogDebug("Msg Queue Check");
                    while (msgQueue.Count > 0)
                    {
                        QueueItem queueItem;
                        lock (msgQueue)
                        {
                            queueItem = msgQueue.Dequeue();
                        }
                        if (queueItem != null)
                            if(!HTTPTransmitEmailItem(queueItem.username, queueItem.filename))
                                Thread.Sleep(5000);
                    }
                }
                catch (Exception ex)
                {
                }
            }
        }
  1. In the code above, why are Thread.Sleep(10000) and Thread.Sleep(5000) used in lines 7 and 18?
  2. Also, why is there a while(true) in line 3?
kovac
  • 4,945
  • 9
  • 47
  • 90
  • 3
    Nobody can answer this but the author of that code. What we can tell you is that this code looks like a mockup or something written by a brand new student. This is not production quality code by any stretch of the imagination. – Kevin Krumwiede Nov 21 '17 at 05:14
  • 2
    I'd argue that two loops are unnecessary here. But `while(true)` is clearly so that it loops while the program is running. Personally, I'd prefer it to loop on a cancellation token status. Not to mention that there's better (async) ways of doing this altogether (a `BlockingCollection`, for example) – ProgrammingLlama Nov 21 '17 at 05:15
  • @john, thanks. any chance you could provide a refactored better version using `BlockingCollection`? – kovac Nov 21 '17 at 05:19
  • 3
    Author wants the code to run indefinitely, hence the `while(true)` loop. Every iteration, it first makes the program sleep for 10 seconds, this is to probably not to overload the system. Then if the system could not `!HTTPTransmitEmailItem`, make it sleep again, here I'm not too sure what the intention is. – JohanP Nov 21 '17 at 05:23

2 Answers2

2

As you requested, here is a slightly better way of doing it:

private static System.Collections.Concurrent.BlockingCollection<MsgType> msgQueue = new System.Collections.Concurrent.BlockingCollection<MsgType>();

private static void AddQueueItems() // simulate adding items to the queue
{
    msgQueue.Add(new MsgType());
    msgQueue.Add(new MsgType());
    msgQueue.Add(new MsgType());
    msgQueue.Add(new MsgType());

    // when adding is done, or the program is shutting down
    msgQueue.CompleteAdding();
}

private static void HTTPProcessQueue()
{
    foreach (var queueItem in msgQueue.GetConsumingEnumerable())
    {
        if (queueItem != null)
        {
            if (!HTTPTransmitEmailItem(queueItem.username, queueItem.filename))
            {
                Thread.Sleep(5000);
            }
        }
    }
}

I'd recommending using the async/await pattern with HTTPTransmitEmailItem, and then you can use await Task.Delay(...) instead of Thread.Sleep(...). I've also not included any error checking in this code.

This would then look more like:

private static async Task HTTPProcessQueue()
{
    foreach (var queueItem in msgQueue.GetConsumingEnumerable())
    {
        if (queueItem != null)
        {
            if (!(await HTTPTransmitEmailItemAsync(queueItem.username, queueItem.filename)))
            {
                await Task.Delay(5000);
            }
        }
    }
}

But you would have to make a HttpTransmitEmailItemAsync method. Also note that the GetConsumingEnumerable(...) method has an overload which takes a CancellationToken, so you could use this to gain more control over when to end the queue process. You can learn about async/await here.

ProgrammingLlama
  • 36,677
  • 7
  • 67
  • 86
  • Any reason for the downvote? He requested that Is how him a better way, and in my answer I have addressed ways that he can make this better again. What's the issue? – ProgrammingLlama Nov 21 '17 at 08:49
0

  1. The Thread.Sleep(10000) is used on line 7 to let the system pause / wait for 10 seconds before it starts the function Utils.LogDebug("Msg Queue Check"); to log the debug information with message "Msg Queue Check". and i believe the Thread.Sleep(5000) is added at the end of loop to create a delay or to wait for 5 seconds before process the next loop.

  2. while(true) is usually used for infinite loop. all method inside this loop will run in loop in infinite time.

mdsuffian
  • 33
  • 4