current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

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

share|improve this question
    
it is still possible with Task.WhenAny method [stackoverflow.com/questions/25683980/… – user3088377 Jul 28 at 18:26
up vote 2 down vote accepted 
private async Task<MyResult> 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.

share|improve this answer
    
Thank you for the detailed response. I will update the code with your suggestions. Reading it back I understand how that comment might not make sense.. the timedOut function takes the request number and adds that to a generic Request XXXX Timed Out.. message. Glad there are just semantic things to fix and the code itself is not bad :) thanks. – Jake Dec 7 '15 at 21:06
up vote 2 down vote

To cancel task when it times out you have to pass a cancellation token to your async operation and then check it periodically inside your PerformCheckAsync method. You should also specify timeout value on CancellationTokenSource. Msdn has a decent article regarding this topic.

var cts = new CancellationTokenSource();
try
{
    cts.CancelAfter(timeout);
    await Client.PerformCheckAsync(request, cts.Token);
}
catch (OperationCanceledException)
{
    //handle cancellation
}
catch (Exception)
{
    //handle exception
}
share|improve this answer
    
Thanks, I did read that article, but I have no control over the PerformCheckAsync method as it is a remote service call, and it does not have any implementations that take 2 params. Does that mean I cannot cancel it? – Jake Dec 7 '15 at 21:01
    
@Jake, pretty much. You can not abort a task from the outside, it needs to implement a cacellation mechanism for you to cancel it. If you have no control over Client impelementation the possible workaround is to move your service call to separate process. This way you can call Process.Kill pretty safely if the operation times out. – Nikita B Dec 8 '15 at 7:41
    
Thanks for the info. I will try moving it to a separate process. – Jake Dec 8 '15 at 21:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.