Destination array was not long enough. when when using Task.WhenAll and AddRange responses to a list

Question

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.

Don't use such code in the first place. Don't try to modify a `List` from multiple threads and *definitely* don't try to use `Select` to cause side-effects. That's at least two critical bugs. Whatever you're trying to do there are better ways — Panagiotis Kanavos, Jun 16 '22 at 08:35
Looks like the exception is originating from `Coach.Domain.Queries.GetRecipeLogsQueryHandler`. Is there some relevant code that gets the `recipes` parameter missing here? — Barry O'Kane, Jun 16 '22 at 08:36
Don't try to reduce multiple complex operations in a single call either. Method chaining is nice up to a point. In this case though, it results in hard-to read code (where does LINQ end and the HTTP call start?) and makes it a lot harder to debug and add the inevitable error handling and retry code necessary in all remote calls — Panagiotis Kanavos, Jun 16 '22 at 08:37
@BarryO'Kane the error is caused because multiple threads are trying to modify the same, thread-unsafe `List<>` — Panagiotis Kanavos, Jun 16 '22 at 08:38
I think your code can overlap since you are writing all your patients to one single array from multiple threads. I suppose this creates the possibility of adding to a list that is currently being modified. I would suggest to get all the objects separately first and after all of them have been completed add them to the list. — Roe, Jun 16 '22 at 08:39
@BarryO'Kane not at all it's working most of the time and the recipes parameter is a list gotten from the database — sina fayazi, Jun 16 '22 at 08:45
Thank you, @PanagiotisKanavos , for your clear reply. I will Edit my code and post. — sina fayazi, Jun 16 '22 at 09:20
If you want to call `_patientService.GetPatientsByClientIds` concurrently, you have to ensure that service is thread safe. If not - create new service for each request. — Svyatoslav Danyliv, Jun 16 '22 at 10:16

score 2 · Answer 1 · answered Jun 17 '22 at 01:07

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.

score 0 · Answer 2 · answered Jun 16 '22 at 12:11

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>.

Destination array was not long enough. when when using Task.WhenAll and AddRange responses to a list

2 Answers2

Other secondary points