-1
\$\begingroup\$

I am very new to Tasks and therefore new to handling task cancellations and task exceptions.

I wrote this method that gets a List which is then used to populate an ObsservableCollection shown in a ListView but I am not sure if I am handling cancellations and exceptions properly.

I would like to cancel awaited tasks if they dont execute successfuly within 10 seconds time frame. In addition, I would like to handle any exceptions and to properly release any resources, hence I am using the "using" statements below.

public override async Task<IList<MyModel>> GetDataAsync(string Id)
{
    try
    {
        // 1. Connecting TcpClient may take up to 90sec?  How to time that to 10sec?
        using (TcpClient client = new TcpClient(ip, port))
        using (NetworkStream stream = client.GetStream())
        {
            byte[] messageBytes = GetMessageBytes(Id);

            using (var writeCts = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
            {
                await stream.WriteAsync(messageBytes, 0, messageBytes.Length, writeCts.Token);
                await stream.FlushAsync();  // 2. Do I even need this?
            }

            byte[] buffer = new byte[1024];
            StringBuilder builder = new StringBuilder();
            int bytesRead = 0;

            using (var readCts = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
            {
                while (stream.DataAvailable)
                {
                    bytesRead = await stream.ReadAsync(buffer, 0, buffer.Length, readCts.Token);
                    builder.AppendFormat("{0}", Encoding.ASCII.GetString(buffer, 0, bytesRead));
                }
            }

            string msg = receivedMessage.ToString();
        }

        return ParseMessageIntoList(msg);  // parses message into IList<MyModel>
    }
    catch (OperationCancelledException ex1)
    {
        // 3. If 10 second timeout expires, I expect to hit this catch block but I dont see it happen?
        Debug.WriteLine(ex.Message);
    }
    catch (Exception ex2)
    {
        // Catch any other exception and return empty list
        return await Task.FromResult<IList<MyModel>>(new List<MyModel>());
    }
}
\$\endgroup\$

closed as off-topic by Heslacher, Peter Taylor, Toby Speight, t3chb0t, Ludisposed Feb 28 at 14:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Heslacher, Peter Taylor, t3chb0t, Ludisposed
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ This code doesn't compile: there are references to variables outside their scope or which simply haven't been defined. This site is for reviewing working code, not debugging broken code or giving advice in the abstract. \$\endgroup\$ – Peter Taylor Feb 28 at 9:49
  • \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Feb 28 at 9:56
1
\$\begingroup\$

You are handling the exceptions correctly from the look of it. You are just missing the code telling it to throw an exception if the token is cancelled (times out in this case).

Try this. I think it should throw as you expect.

using (var writeCts = new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(10)))
{
    writeCts.Token.ThrowIfCancellationRequested();
    await stream.WriteAsync(messageBytes, 0, messageBytes.Length, writeCts.Token);
    await stream.FlushAsync();  // 2. Do I even need this? I think so. This ensures it has finished writing to the actual hardware.
}

see here for how to set a connect timeout on your tcpclient

https://stackoverflow.com/questions/17118632/how-to-set-the-timeout-for-a-tcpclient

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! As it currently stands, your answer does not contain a review. Please provide at least one insightful remark about the code and/or approach provided by the author. \$\endgroup\$ – Mast Feb 28 at 3:03

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