Closing WCF Service from Async method?

Question

I have a service layer project on an MVC 5 ASP.NET application I am creating on .NET 4.5.2 which calls out to an External 3rd Party WCF Service to Get Information asynchronously. An original method to call external service was as below (there are 3 of these all similar in total which I call in order from my GetInfoFromExternalService method (note it isnt actually called that - just naming it for illustration)

    private async Task<string> GetTokenIdForCarsAsync(Car[] cars)
    {
        try
        {
            if (_externalpServiceClient == null)
            {
                _externalpServiceClient = new ExternalServiceClient("WSHttpBinding_IExternalService");
            }

            string tokenId= await _externalpServiceClient .GetInfoForCarsAsync(cars).ConfigureAwait(false);

            return tokenId;
        }
        catch (Exception ex)
        {
            //TODO plug in log 4 net 
            throw new Exception("Failed" + ex.Message);
        }
        finally
        {
            CloseExternalServiceClient(_externalpServiceClient);
            _externalpServiceClient= null;
        }
    }

So that meant that when each async call had completed the finally block ran - the WCF client was closed and set to null and then newed up when another request was made. This was working fine until a change needed to be made whereby if the number of cars passed in by User exceeds 1000 I create a Split Function and then call my GetInfoFromExternalService method in a WhenAll with each 1000 - as below:

if (cars.Count > 1000)
        {
            const int packageSize = 1000;
            var packages = SplitCarss(cars, packageSize);

            //kick off the number of split packages we got above in Parallel and await until they all complete
            await Task.WhenAll(packages.Select(GetInfoFromExternalService));
         }

However this now falls over as if I have 3000 cars the method call to GetTokenId news up the WCF service but the finally blocks closes it so the second batch of 1000 that is attempting to be run throws an exception. If I remove the finally block the code works ok - but it is obviously not good practice to not be closing this WCF client.

I had tried putting it after my if else block where the cars.count is evaluated - but if a User uploads for e.g 2000 cars and that completes and runs in say 1 min - in the meantime as the user had control in the Webpage they could upload another 2000 or another User could upload and again it falls over with an Exception.

Is there a good way anyone can see to correctly close the External Service Client?

Answer 1

Based on the related question of yours, your "split" logic doesn't seem to give you what you're trying to achieve. WhenAll still executes requests in parallel, so you may end up running more than 1000 requests at any given moment of time. Use SemaphoreSlim to throttle the number of simultaneously active requests and limit that number to 1000. This way, you don't need to do any splits.

Another issue might be in how you handle the creation/disposal of ExternalServiceClient client. I suspect there might a race condition there.

Lastly, when you re-throw from the catch block, you should at least include a reference to the original exception.

Here's how to address these issues (untested, but should give you the idea):

const int MAX_PARALLEL = 1000;
SemaphoreSlim _semaphoreSlim = new SemaphoreSlim(MAX_PARALLEL);

volatile int _activeClients = 0;
readonly object _lock = new Object();
ExternalServiceClient _externalpServiceClient = null;

ExternalServiceClient GetClient()
{
    lock (_lock)
    {
        if (_activeClients == 0)
            _externalpServiceClient = new ExternalServiceClient("WSHttpBinding_IExternalService");
        _activeClients++;
        return _externalpServiceClient;
    }
}

void ReleaseClient()
{
    lock (_lock)
    {
        _activeClients--;
        if (_activeClients == 0)
        {
            _externalpServiceClient.Close();
            _externalpServiceClient = null;
        }
    }
}

private async Task<string> GetTokenIdForCarsAsync(Car[] cars)
{
    var client = GetClient();
    try 
    {
        await _semaphoreSlim.WaitAsync().ConfigureAwait(false);
        try
        {
            string tokenId = await client.GetInfoForCarsAsync(cars).ConfigureAwait(false);
            return tokenId;
        }
        catch (Exception ex)
        {
            //TODO plug in log 4 net 
            throw new Exception("Failed" + ex.Message, ex);
        }
        finally
        {
            _semaphoreSlim.Release();
        }
    }
    finally
    {
        ReleaseClient();
    }
}

Updated based on the comment:

the External WebService company can accept me passing up to 5000 car objects in one call - though they recommend splitting into batches of 1000 and run up to 5 in parallel at one time - so when I mention 7000 - I dont mean GetTokenIdForCarAsync would be called 7000 times - with my code currently it should be called 7 times - i.e giving me back 7 token ids - I am wondering can I use your semaphore slim to run first 5 in parallel and then 2

The changes are minimal (but untested). First:

const int MAX_PARALLEL = 5;

Then, using Marc Gravell's ChunkExtension.Chunkify, we introduce GetAllTokenIdForCarsAsync, which in turn will be calling GetTokenIdForCarsAsync from above:

private async Task<string[]> GetAllTokenIdForCarsAsync(Car[] cars)
{
    var results = new List<string>();
    var chunks = cars.Chunkify(1000);
    var tasks = chunks.Select(chunk => GetTokenIdForCarsAsync(chunk)).ToArray();
    await Task.WhenAll(tasks);
    return tasks.Select(task => task.Result).ToArray();
}

Now you can pass all 7000 cars into GetAllTokenIdForCarsAsync. This is a skeleton, it can be improved with some retry logic if any of the batch requests has failed (I'm leaving that up to you).