I'm using Task.WhenAll to call multiple async requests with different parameters to get patients of various pharmacies by selecting patient ids of that pharmacy. And I want to add this data to one list and then map it to other models.

Here is my code.

var patients = new List<PatientDTO>();
            await Task.WhenAll(
                pharmacyIds.Select(async pharmacyId =>
                    patients.AddRange(await _patientService.GetPatientsByClientIds(pharmacyId, recipes
                            .Where(x => x.PharmacyId == pharmacyId)
                            .Select(x => x.PatientId)))));

I'm getting this data paginated in my request, and my page size is often 50 most of the time, this works correctly, but sometimes it throws an exception with the message: Destination array was not long enough. Check the destination index, length, and the array's lower bounds. (Parameter 'destinationArray').

Here is some more information about the exception.

at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length, Boolean reliable)
   at System.Collections.Generic.List`1.Insert(Int32 index, T item)
   at System.Collections.Generic.List`1.InsertRange(Int32 index, IEnumerable`1 collection)
   at Coach.Domain.Queries.GetRecipeLogsQueryHandler.<>c__DisplayClass4_0.<<Handle>b__14>d.MoveNext()

Where is my mistake, and how can I solve this?

• NOTE: I want multiple requests to be async, and I don't want to call them in sequence.

CodePudding user response：

If you can change GetPatientsAsync having a version of GetPatientsAsync that takes a collection of (pharmacyId, recipes) and returns the patient ids should be significantly more performant than paralleling the requests to GetOne.

This would be a difference between a single query vs multiple trips to the database and having a single query would be much faster.

Task<List<int>> GetPatientsAsync(
   IEnumerable<(string pharmancyId, IEnumerable<Prescription> prescriptions)> inputs)

//Or (because prescriptions don't seem to change in your "loop")
Task<List<int>> GetPatientsAsync(
   IEnumerable<string> pharmancyIds, IEnumerable<Prescription> prescriptions) 

Other secondary points

• GetPatientsByClientIds has ByClientIds but doesn't take in client ids
• GetPatientsXXX should be GetPatientsAsync (suffix)
• If you have to .Where(x => x.PharmacyId == pharmacyId) after calling GetPatientsByClientIds(pharmacyId, recipes)) - which takes pharmacyId - seems to suggest that something is not right.

CodePudding user response：

Retrieving the patients as a side-effect of the Select lambda execution is not a good idea, because it makes your program fragile and difficult to understand. If you insist at doing it, you should at least protect the mutation of the patients list with a lock:

var patients = new List<PatientDTO>();
await Task.WhenAll(
    pharmacyIds.Select(async pharmacyId =>
    {
        var sublist = (await _patientService.GetPatientsByClientIds(pharmacyId, recipes))
                .Where(x => x.PharmacyId == pharmacyId)
                .Select(x => x.PatientId)
                .ToList();
        lock (patients) patients.AddRange(sublist);
    }));

It is important to release the lock as soon as possible, so the sublist must be a materialized collection (List<T>, array etc). Otherwise, if it's a non-materialized enumerable, the AddRange will have to do a lot of work, it'll take a lot of time, harming your goal to parallelize the operations.

A better approach is to remove the side-effects, and merge the patients after the completion of all the tasks. Example:

Task<List<int>>[] tasks = pharmacyIds
    .Select(async pharmacyId =>
    {
        return (await _patientService.GetPatientsByClientIds(pharmacyId, recipes))
            .Where(x => x.PharmacyId == pharmacyId)
            .Select(x => x.PatientId)
            .ToList();
    }).ToArray();

List<int>[] results = await Task.WhenAll(tasks);

List<int> patients = results.SelectMany(x => x).ToList();

An even better approach IMHO is to switch from the Task.WhenAll to the new (.NET 6) Parallel.ForEachAsync API, because it allows you to configure the degree of parallelism. It has the disadvantage that it doesn't propagate the results (workaround here), so you'll have to use the lock AddRange technique from the first example, or a concurrent collection like the ConcurrentQueue<T>.