2

I'm creating 10 tasks and every task creates a number with increment. Then i enqueue to ConcurrentQueue As a result i there are 10 numbers in the queue but every numbers value is 10.

How can I set every numbers in queue have different value (0 to 9). Also the TaskTest() method should complete in less than 10 second.

public async void TaskTest()
{
    ConcurrentQueue<int> queue;
    queue = await GetNumbers();
}

private async Task<ConcurrentQueue<int>> GetNumbers()
{
    ConcurrentQueue<int> queue = new ConcurrentQueue<int>();
    List<Task> tasks = new List<Task>();

    int i = 0;
    while (i<10)
    {
        tasks.Add(Task.Factory.StartNew(() =>
        {
            var number = CreateNumber(i);
            queue.Enqueue(number);
        }));
        Interlocked.Increment(ref i);
    }
    
    foreach (var t in tasks)
    {
        await t;
    }
    return queue;
}

private int CreateNumber(int i)
{
    Thread.Sleep(1000);
    return i;
}

The result:

enter image description here

Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
Tolga Cakir
  • 725
  • 1
  • 7
  • 13
  • 1
    You should be careful about accidentally modifying captured variables like `i` after starting the thread, because the `i` is shared. Here is the solution to your question https://stackoverflow.com/a/34319356/2946329 – Salah Akbari Sep 25 '20 at 07:51
  • You will need to capture `i` for each task. See [this answer](https://stackoverflow.com/a/25973952/1997232). – Sinatr Sep 25 '20 at 07:55
  • Can't find good duplicate, the best I found is [this](https://stackoverflow.com/q/3168375/1997232). – Sinatr Sep 25 '20 at 07:58
  • 2
    You shouldn't use `Task.Factory.StartNew`, use [`Task.Run`](https://stackoverflow.com/a/38423507/542251). I'd also recommend you use [`Task.Delay`](https://stackoverflow.com/a/20084603/542251) not `Thread.Sleep` as this can then unblock the thread – Liam Sep 25 '20 at 08:21
  • 2
    Solved. Thanks for your answer @SalahAkbari. Also I consider your answer Fildor – Tolga Cakir Sep 25 '20 at 08:22
  • 1
    and use [`Task.WhenAll`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.whenall?view=netcore-3.1) not your `for` loop. Basically throw all this away and start again using proper up to date TPL – Liam Sep 25 '20 at 08:24
  • 1
    I'd *also* mention that that duplciate answer is 5 years old and is also out of date. In 2020 you should never have to manipulate the `Thread` directly. Pretty much always use the `Task` abstraction as it gives you asynchronicity – Liam Sep 25 '20 at 08:27
  • How to write basically this code with your recommended way? @Liam. I don't have more information about TPL – Tolga Cakir Sep 25 '20 at 08:33
  • They closed the question just as I wanted to post answer - you can create a situation where a new variable will be captured by each of the created Task items (threads), like this:while (i < 10){int temp = i;tasks.Add(Task.Factory.StartNew(() =>... - it give numbers as you expected and finishes in <10 sec – Ivan Golović Sep 25 '20 at 08:38
  • 1
    The return value of `Interlocked.Increment` is important in your case. It is the only way to know the current value of `i` with thread-safety: `int localIndex = Interlocked.Increment(ref i);` – Theodor Zoulias Sep 25 '20 at 09:04
  • Are the queued values supposed to be consumed while those tasks are queueing them? – Paulo Morgado Sep 25 '20 at 09:22

1 Answers1

1

Regarding while (i<10), if you know the number iterations up front, you should really be using a for loop rather than a while.

Using a for loop or Enumerable.Range you don't need to worry about manipulating i in a thread safe way:

private async Task<ConcurrentQueue<int>> GetNumbers()
{
    ConcurrentQueue<int> queue = new ConcurrentQueue<int>();

    Task EnqueOnThreadPoolAsync(int i) => Task.Run(() => queue.Enqueue(CreateNumber(i)));

    await Task.WhenAll(Enumerable.Range(0, 10).Select(EnqueOnThreadPoolAsync));

    return queue;
}
Johnathan Barclay
  • 18,599
  • 1
  • 22
  • 35