Async telnet connection over StreamSocket

Question

My team and I have a very poor understanding of best practices in relation to telnet. We have Task.Delay and Task.Wait in the code, including async voids and from what I understand those are potential areas that can cause deadlocks and other issues.

I'm trying to understand async in relation to running a telnet client. Is this a safe implementation of cancellation tokens and telnet connections?

public enum TelnetClientStatus : int
{
 Unset = 0,
 Connected= 1,
 Disconnected = 2,
 Failed = 3
}
public TelnetClientStatus Status { get; set; } = TelnetClientStatus.Unset;
public async Task OpenConnection()
{
 //Don't allow the session to keep trying to open after the 5 seconds
 var cancellationTokenSource = new CancellationTokenSource();
 cancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(5));
 Status = await Task.Run(MakeConnection, cancellationTokenSource.Token);
}
private async Task<TelnetClientStatus> MakeConnection()
{
 _streamSocket = new StreamSocket();
 _dataWriter = new DataWriter(_streamSocket.OutputStream);
 _dataReader = new DataReader(_streamSocket.InputStream)
 {
 InputStreamOptions = InputStreamOptions.Partial
 };
 try
 {
 await _streamSocket.ConnectAsync(new HostName(HostName), Port.ToString());
 return TelnetClientStatus.Connected;
 }
 catch (TaskCanceledException) //All other exceptions need to be investigated not ignored
 {
 //I understand a connection failed due to timeout, give the app some flag to work with
 //Maybe retry the connection?
 return TelnetClientStatus.Failed;
 }
 catch (Exception ex)
 {
 throw ex;
 }
}

Answer 1

I see at least two points.

• if you have a catch to catch a sytem Exception (wich is not good btw., it would be better to catch specific exceptions) and the only action is to throw that exception then its useless to have. If you need to rethrow an exception you should always omit the ex so just throw;, otherwise the StackTrace will be broken.

• Having the public setter of the Status property is breaking encapsulation. Why should a user of the object be able to set this property? You should make it private.

• C Bauer: Unfortunately the exception that gets thrown is actually of type system exception and I have no way of being more specific.
• Bobson: @CBauer - Then don't catch it. If you can't handle an exception, don't try.
• C Bauer: @Bobson What do you mean? The exception is a generic async error of type System.Exception and occurs when one async call returns before a previous call due to network latency. The right thing to do in that situation is to ignore the response from the earlier call.
• Bobson: @CBauer - If the right thing to do is ignore it, then you should swallow the error with a comment explaining why (like you do with TaskCanceledException). You don't have to catch Exception as the last thing in your try/catch block. Either do something with it or don't catch it. Never just catch and rethrow without a very specific reason.
• C Bauer: Wires crossed my mistake I was thinking of different code! I have removed the errant catch from the code.