HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Async task with timeout

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asynctimeoutwithtask

Problem

I am calling a service outside of my control. My application must include a time out, so that if the call to the service takes too long, an appropriate time-out message is returned.

// Client connected to remote service
RemoteClient Client = new RemoteClient();

private async Task CheckItAsync(CheckRequest rqst, int timeout)
{
    // MyResult object gets some values populated on construction
    MyResult Result = new MyResult()

    // RemoteResponse auto generated from 'Add Service Reference'
    RemoteResponse response;

    try
    {
        // Perform the check and return the appropriate response.
        var task = Client.PerformCheckAsync(rqst);
        var res = await Task.WhenAny(task, Task.Delay(timeout));
        if (res == task)
        {
            // Task completed within time.
            response = task.GetAwaiter().GetResult();
            // Populate my result object with the necessary values
            Result.populate(response);
        }
        else
        {
            // Task timed out, populate result with timeout message
            Result.timedOut(rqst.RequestNumber);
        }

    }
    catch (Exception e)
    {
        // Something else happened, log exception and return failed result
        Elmah.ErrorSignal.FromCurrentContext().Raise(e);
        Result.failed(rqst.RequestNumber);
    }

    return Result;
}


I read that Task.WhenAny.. will still continue calling the service in the background even after the time out, creating a leak, but I am not sure how to stop the call as the remote service does not take a CancellationToken.

The code works fine and returns the correct responses when it times out and when it doesn't.

*Additional follow-up question, does this code call the remote service multiple times? (at both var task = ... and Task.WhenAny...)

Solution

private async Task CheckItAsync(CheckRequest rqst, int timeout)


Never use abbreviations neither for method arguments not for variable/method/class names. A few more characters to make the name meaningful doesn't slow down the code nor does it increase the code size in a bad way. In fact you are saving yourself a lot of time if you come back in a few weeks/months to fix some bugs because you see at first glance what it is about.

MyResult Result = new MyResult()


This is a classname which doesn't tell what the class is about. Sure it will be some kind of result of some kind of operation but it neither tells what kind of result this is nor from which operation that result is coming. You could name it Result and it would have the same meaning (both times no meaning).

Variables names should be named using camelCase casing based on the .NET naming guidelines.

RemoteResponse response;


Variables should be placed as near to their usage as possible. Here you have placed this outside of the try..catch but you use it only in the try block.

Result.populate(response);


Methods should be named using PascalCase casing, hence populate should become Populate. The same is true for the other MyResult methods.

// Task timed out, populate result with timeout message
Result.timedOut(rqst.RequestNumber);


Comments should tell the reader why something is done in the way it is done. What is done should be told by the code itself using meaningful names for classes, methods, properties and variables. Furthermore comments shouldn't lie like this comment is doing. The result isn't populated with a timeout message but with the RequestNumber of the CheckRequest.

Because methodnames should be made out of verbs or verb phrases timedOut isn't a good name for a method.

Code Snippets

private async Task<MyResult> CheckItAsync(CheckRequest rqst, int timeout)
MyResult Result = new MyResult()
RemoteResponse response;
Result.populate(response);
// Task timed out, populate result with timeout message
Result.timedOut(rqst.RequestNumber);

Context

StackExchange Code Review Q#113108, answer score: 4

Revisions (0)

No revisions yet.