CODEWITHSUNDEEP
c#.netmultithreadingtask-parallel-librarytask
Bes Ley
Bes Ley

Reputation: 1797

Multithreading Task read a list, caused index out of range

There is a case that the application system needs publish messages to online users every 1 minutes, and the code is using multithead tasks to read a message list. But the program is not run correctly, it will throw an index out of range exception. please someone could make any sugguestions, thanks.

private Timer taskTimer;
private static readonly object _locker = new object();
private static IList<Message> _messages = null;

private void OnTimerElapsed(object sender)
{
    var msgModel = new MessageModel();
    _messages = msgModel.GetMessageList();
    var msgCount = _messages.Count();

    Task[] _tasks = new Task[msgCount];
    for (int i = 0; i < msgCount; i++)
    {
        if (i < msgCount)
        {
            _tasks[i] = Task.Factory.StartNew(() =>
            {
                lock (_locker)
                {
                    PushMessage(i);
                }
            });
        }
    }

    //waiting all task finished
    while (_tasks.Any(t => !t.IsCompleted)) { }
}

private void PushMessage(int i)
{          
    var msg = _messages[i];         //it will throw an exception here...
    //send message to on line users.
    SendToOnlineUsers(msg);
}

Error:
Index was out of range. Must be non-negative and less than the size of the collection.

StackTrace Details:
   at System.ThrowHelper.ThrowArgumentOutOfRangeException()
   at System.Collections.Generic.List`1.get_Item(Int32 index)
   at WebIM.Hubs.BackgroudPushServiceTimer.PushMessage(Int32 i) in ...
   at WebIM.Hubs.BackgroudPushServiceTimer.<>c__DisplayClass6.<OnTimerElapsed>b__2() in ...
   at System.Threading.Tasks.Task.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()

If the messages count is 4, and the index will be 4 too in the PushMessage function, it is out of range.

Upvotes: 2

Views: 1983

Answers (1)

Jon Skeet
Jon Skeet

Reputation: 1502656

The problem is that you're capturing i, a variable whose value changes over time. You can fix it by making a local copy inside the loop:

for (int i = 0; i < msgCount; i++)
{
    int copyOfI = i;
    if (i < msgCount)
    {
        _tasks[i] = Task.Factory.StartNew(() =>
        {
            lock (_locker)
            {
                PushMessage(copyOfI);
            }
        });
    }
}

That said, I suspect there are cleaner ways of handling this - especially as you're creating several tasks which are all using the same lock. You're not actually achieving any concurrency here, and then you're waiting for everything to finish - so why are you going to all of this trouble at all?

You should also look at Task.WhenAll / Task.WaitAll as a much more efficient way of waiting for tasks to complete.

EDIT: If you can do this in parallel, consider using Parallel.For or Parallel.ForEach instead.

Upvotes: 5

Related Questions