0

When i execute the code, inspecting after WhenAll the idList is empty, if i step through it using breakpoints after the WhenAll everything works as expected.

This code is within a non-async method.

Its as if letting it run on its own is not starting the tasks.

var currentPage = 1;
List<Result> idList = new List<Result>();

var response = this.GetDocuments(currentPage).Result;
//idList.AddRange(response.results);

var tasks = new List<Task>();

for (var i = 0; i < response.meta.page.total_pages; ++i)
{
    var t = new Task(() =>
    {
        response = this.GetDocuments(i+1).Result;
        idList.AddRange(response.results);
    });
    tasks.Add(t);
    t.Start();
}

 Task.WhenAll(tasks).Wait();

 public async Task<ResponseObject> GetDocuments(int currentPage)
        {
            var result = await httpClient.GetAsync($"api/as/v1/engines/urlname/documents/list?page[current]={currentPage}").ConfigureAwait(false);

            if (!result.IsSuccessStatusCode)
            {
                loggly.Error($"Exception: ScheduledJobBase, IndexPages, failed when getting documents from Swiftype, response: { result }");
                return null;
            }

            return await result.Content.ReadAsAsync<ResponseObject>().ConfigureAwait(false); ;
        }
Hawkzey
  • 1,088
  • 1
  • 11
  • 21
  • 3
    The expression `Task.WhenAll(tasks).ConfigureAwait(false)` returns a `Task`, you need to `await` it (or call `.Wait()` in non-async method). – tukaef May 30 '19 at 12:12
  • 1
    Possible duplicate of [Task.WhenAll not waiting](https://stackoverflow.com/questions/36905187/task-whenall-not-waiting) – Liam May 30 '19 at 12:15
  • I have made this modification but the same result is still happening i will update question. @tukaef – Hawkzey May 30 '19 at 12:17
  • Another thing - you're reassigning `response` in each task, that might cause some problems. – tukaef May 30 '19 at 12:20
  • The response is coming from an api call, so this is needed to get the new data. @tukaef I have added the function. – Hawkzey May 30 '19 at 12:22
  • 2
    You're also passing the loop variable (`i`) into the task's lambda which creates a closure, so your `GetDocuments` calls _might_ use wrong values for `i`. In order to fix this, you could create an intermediate variable before creating a task. Like `var i1 = i; ... GetDocuments(i1+1) ...` – tukaef May 30 '19 at 12:24
  • This fixed it, could you explain or guide me to something that explains this closure? and why it has happened, thank you for the fix. @tukaef – Hawkzey May 30 '19 at 12:32
  • 1
    You can start with this [answer](https://stackoverflow.com/a/271447/1009661) – tukaef May 30 '19 at 12:47
  • 1
    Yet another point - as @Hakit01 mentioned, accessing a non-concurrent collection from multiple threads might cause a lot of nasty bugs. So you can use a concurrent collection (`System.Collections.Concurrent` namespace) or, in your case, I'd avoid using a collection at all. `Task.WhenAll` creates a task returning all the results of underlying tasks, so returning a value from your tasks then aggregating them into a collection would solve the problem. Also, you needn't create the tasks manually - just extract an async method from the lambda you put in the task constructor. – tukaef May 30 '19 at 13:18
  • 1
    A few points: 1) Don't use `Wait` or `Result`; use `await` instead. There are very few exceptions to this rule. 2) Never, ever, ever use the `Task` constructor. There are **no** exceptions to this rule. 3) Return results instead of updating a shared collection. – Stephen Cleary May 30 '19 at 13:53
  • @StephenCleary how do i avoid using the task constructor in this case?, and also i used result because this was inside a non-async function so i could not use await, is there a better approach?. Also i am using a shared collection because the api is limited to 100 documents and i need to get all the documents before doing a delete. – Hawkzey May 30 '19 at 13:55
  • 1
    @Hawkzey: You can call the method for each item in your collection, then `await` them all with `Task.WhenAll`. Sync-over-async is a hack; the ideal solution is to make the calling method asynchronous. – Stephen Cleary May 30 '19 at 21:25
  • @StephenCleary I have changed it to do exactly that, thanks. – Hawkzey May 31 '19 at 07:55

2 Answers2

0

The issue happened might be due to the collection is not synchronized, Might have thrown InvalidAccessException.

Hakit01
  • 91
  • 3
  • 8
  • It's not. [It's this](https://stackoverflow.com/questions/56377995/listtasks-dont-do-anything-when-i-run-code-but-if-i-step-through-it-works#comment99356470_56377995) – Liam May 30 '19 at 12:14
0

Although the ideal scenario would be to convert the method return type to a Task and return or await the result of Task.WhenAll, if the method must remain non-async, then call .Wait() on the result of the Task.WhenAll call like so:

Task.WhenAll(tasks).Wait();
Craig Smitham
  • 3,395
  • 4
  • 31
  • 38
  • I have made this change and it doesnt not fix my problem :/, and ConfigureAwait doesnt not contain a wait method?. – Hawkzey May 30 '19 at 12:20
  • 1
    Just call `.Wait()` on Task.WhenAll() (I've updated the answer to reflect this). You shouldn't need ConfigureAwait if you are not using async/await. – Craig Smitham May 30 '19 at 12:34