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

Async TCP client/server

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

Problem

I'm working on a simple async TCP client server application, i'd like my network code to be reviewed. I really feel that I'm doing something wrong, especially in receiving code.

Interface for client:

public delegate void ConnectedHandler(IAsyncClient a);

public delegate void ClientMessageReceivedHandler(IAsyncClient a, List msg);

public delegate void ClientMessageSubmittedHandler(IAsyncClient a, bool close);

public delegate void ClientErrorHandler(string errorMessage);

public interface IAsyncClient : INetworkNode, IDisposable
{
    event ConnectedHandler Connected;
    event ClientMessageReceivedHandler MessageReceived;
    event ClientMessageSubmittedHandler MessageSubmitted;
    event ClientErrorHandler Error;
    void StartClient();
    bool IsConnected { get; }

    Task StartReceiving();
    void Send(IProcessable message, bool close);
}


INetworkNode:

public interface INetworkNode
{
    byte[] WrapMessage(byte[] message);

    byte[] WrapKeepaliveMessage();
}


Wrapper needed for client:

```
internal class PolicyWrapper
{
internal PolicyResult InitiateRetryPolicy(int retryCount, int retyDelaySeconds, Action executionTarget,
Action internalExceptionHandler)
{
var policyResult = Policy
.Handle()
//.Or(ex => ex.ParamName == "example")
.WaitAndRetry(retryCount,
i => TimeSpan.FromSeconds(retyDelaySeconds),
(exception, span) => internalExceptionHandler(exception))
.ExecuteAndCapture(executionTarget);

return policyResult;
}

public void InitiateEndlessRetryPolicy(int retyDelaySeconds, Action executionTarget,
Action internalExceptionHandler)
{
Policy
.Handle()
//.Or(ex => ex.ParamName == "example")
.WaitAndRetryForever(i => TimeSpan.FromSeconds(retyDelaySeconds),
(exception, span) => internalExceptionHandler(exception)).Execute(executionTarget);
}
}

Solution

Async/Await

Async/await's main advantage is that you no longer need to use ContinueWith() to chain tasks, making the code much cleaner. Your code seems to more often favor the use of ContinueWith(). A good book or tutorial on this will help you refactor that part of the code.

For example, this code:

public Task DoSomethingAsync()
{
    return Task.Factory.StartNew(() =>
    {
        //...
        return 1;
    });
}

var task = DoSomethingAsync();
task.ContinueWith(value =>
{
     Console.WriteLine("{0}", value);
});


Becomes

public Task DoSomethingAsync()
{
    return Task.Factory.StartNew(() =>
    {
        //...
        return 1;
    });
}

var value = await DoSomethingAsync();
Console.WriteLine("{0}", value);


Event Notification

In the spirit of asynchronicity, I would rather expose some events which would notify interested parties that something has been sent/received than making them wait on ManualResetEvents. Later I noticed that you also use events. In this case, why did you use the ManualResetEvent? As for the delegate type of the events, using EventHandler<> specializations makes the code more idiomatic.

Single Responsibility Principle

A short look upon StreamSecurityClient and one immediately notices that this class is doing a lot. For example, the client shouldn't be concerned in how a command is handled.

The socket notifies you that some bytes have arrived over the wire. A protocol decoder receives all the bytes and splits them into meaningful messages (as mentioned in a comment, a stream of bytes doesn't mean that you receive a full message every time, it's your task to put together ["He", "llo W", "orld"] into ["Hello", "World"]). After you have a specific message, you pass it on to whatever handling mechanism you choose.

Over-engineering

I can't help noticing the large number of patterns you employ. Are you learning design-patterns? Simply throwing in all the patterns one can think off can easily lead to code that is unnecessarily complex, prepared to solve issues that might never arise at the expense of being harder to follow: YAGNI

Code Snippets

public Task<int> DoSomethingAsync()
{
    return Task.Factory.StartNew(() =>
    {
        //...
        return 1;
    });
}

var task = DoSomethingAsync();
task.ContinueWith(value =>
{
     Console.WriteLine("{0}", value);
});
public Task<int> DoSomethingAsync()
{
    return Task.Factory.StartNew(() =>
    {
        //...
        return 1;
    });
}

var value = await DoSomethingAsync();
Console.WriteLine("{0}", value);

Context

StackExchange Code Review Q#138014, answer score: 6

Revisions (0)

No revisions yet.