I have a TCP Client that mainly runs on mono that I wish some guidance with, I think I am doing some things wrong, something not needed, etc.
The below code is part of what I use to serve as a sample for my doubts.
As you can see once the constructor is called, its when I instantiate the ConcurrentQueues, should I have it instantiated by itself not needing to initiate it from the constructor or the way I am currently doing is the correct one or it doesn't really matter ?
I currently have 3 threds running which I belive I could reduce to 2 or even one but I am a bit insecured for doing that.
As you can see I have:
receiveThreadfor_ReceivePacketsThis one controls all received data from the roomserversendThreadfor_SendPacketsThis one controls everything that must be sent to the roomserverresponseThreadfor_ResponseThis will handles all the responses that were queued from the roomserverI belive I could merge
_SendPacketswith_ReceivePacketsas one and increase to my classSendPacketswether it is a packet to be sent or one that was delivered, what I am afraid of is that if it has a huge in/out if it would still keep up withou messing things up.I have the
_Responseseparated as it will be processing more of the response data per type of reply which I think is fine and don't think it would work out if I remove it and let the_Responsehandle it by itself since some packets won't be readed in just one shot.How far should I rely myself into the
_socket.Connected?I am having some issues to deploy a reconnect, most of the time when I have some connection issue, it doesn't trigger any errors it just sit in there with the ports open as if it still connected, how should I detect if I am still live or not ?
Over all recommendations, advices or online free reading materials ?
Side note: This is a very basic implementation of a chat tcp client for learning that I am currently working on.
using System;
using System.IO;
using System.Net;
using System.Text;
using System.Threading;
using System.Net.Sockets;
using System.Collections.Concurrent;
using log4net;
namespace Connection
{
public class Roomserver
{
private static readonly ILog logger = LogManager.GetLogger(typeof(Roomserver));
private ConcurrentQueue<byte[]> RoomserverReceivedPackets = null;
private ConcurrentQueue<SendPackets> RoomserverSendPackets = null;
private AutoResetEvent _queueNotifier = new AutoResetEvent(false);
private AutoResetEvent _sendQueueNotifier = new AutoResetEvent(false);
public static byte[] myinfo = null;
private IPAddress _server = null;
private int _port = 0;
private int _roomID = 0;
private Socket _socket;
private Status _status = Status.Disconnected;
private Thread responseThread = null;
private Thread receiveThread = null;
private Thread sendThread = null;
private EndPoint _roomServer = null;
public bool Connected
{
get { return _socket.Connected; }
}
public Status GetStatus
{
get { return _status; }
}
public Roomserver(IPAddress server, int port)
{
this._server = server;
this._port = port;
RoomserverReceivedPackets = new ConcurrentQueue<byte[]>();
RoomserverSendPackets = new ConcurrentQueue<SendPackets>();
}
public Roomserver(IPAddress server, int port, int roomID)
{
this._server = server;
this._port = port;
this._roomID = roomID;
RoomserverReceivedPackets = new ConcurrentQueue<byte[]>();
RoomserverSendPackets = new ConcurrentQueue<SendPackets>();
}
public bool Connect()
{
try
{
if (_status != Status.Disconnected)
this.Disconnect();
_socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
IPEndPoint remoteEndPoint = new IPEndPoint(_server, _port);
_socket.Connect(remoteEndPoint);
_status = Status.Connect;
_roomServer = (EndPoint)remoteEndPoint;
receiveThread = new Thread(_ReceivePackets);
receiveThread.Start();
sendThread = new Thread(_SendPackets);
sendThread.Start();
responseThread = 开发者_运维知识库new Thread(_Response);
responseThread.Start();
return _socket.Connected;
}
catch (SocketException se)
{
logger.Error("Connect: " + se.ToString());
_status = Status.Disconnected;
return false;
}
catch (Exception ex)
{
logger.Error("Connect: " + ex.ToString());
_status = Status.Disconnected;
return false;
}
}
public bool Disconnect()
{
if (_socket.Connected)
{
_status = Status.Disconnected;
if (receiveThread != null && receiveThread.IsAlive)
{
receiveThread.Abort();
}
if (responseThread != null && responseThread.IsAlive)
{
responseThread.Abort();
}
if (sendThread != null && sendThread.IsAlive)
{
sendThread.Abort();
}
try
{
_socket.Close();
return true;
}
catch (Exception ex)
{
logger.Info("Disconnect " + ex.ToString());
_status = Status.Disconnected;
return true;
}
}
else
{
logger.Info("Not connected ...");
_status = Status.Disconnected;
return true;
}
}
public bool SendData(byte[] bytes, bool delay)
{
try
{
SendPackets data = new SendPackets()
{
Data = bytes,
Delay = delay
};
RoomserverSendPackets.Enqueue(data);
_sendQueueNotifier.Set();
return true;
}
catch (Exception ex)
{
logger.Error("SendData " + ex.ToString());
return false;
}
}
private void _SendPackets()
{
while (_socket.Connected)
{
_sendQueueNotifier.WaitOne();
while (!RoomserverSendPackets.IsEmpty)
{
SendPackets packet = null;
if (RoomserverSendPackets.TryDequeue(out packet))
{
try
{
if (packet.Delay)
{
Thread.Sleep(1000);
_socket.Send(packet.Data);
}
else
_socket.Send(packet.Data);
}
catch (SocketException soe)
{
logger.Error(soe.ToString());
}
}
}
}
}
private void _ReceivePackets()
{
bool extraData = false;
MemoryStream fullPacket = null;
int fullPacketSize = 0;
while (_socket.Connected)
{
try
{
byte[] bytes = new byte[65536];
int bytesRead = _socket.ReceiveFrom(bytes, ref _roomServer);
int packetSize = 0;
int reply = 0;
byte[] data = new byte[bytesRead];
Array.Copy(bytes, data, bytesRead);
MemoryStream bufferReceived = new MemoryStream(data, 0, data.Length);
using (var reader = new BinaryReader(bufferReceived))
{
packetSize = (int)reader.ReadInt32() + 4;
reply = (int)reader.ReadByte();
}
if (!extraData && packetSize <= bytesRead)
{
if (data.Length > 0)
{
RoomserverReceivedPackets.Enqueue(data);
_queueNotifier.Set();
}
}
else
{
if (!extraData)
{
fullPacket = new MemoryStream(new byte[packetSize], 0, packetSize);
fullPacket.Write(data, 0, data.Length);
fullPacketSize = data.Length;
extraData = true;
}
else
{
if (fullPacketSize < fullPacket.Length)
{
int left = (int)fullPacket.Length - fullPacketSize;
fullPacket.Write(data, 0, (left < data.Length) ? left : data.Length);
fullPacketSize += (left < data.Length) ? left : data.Length;
if (fullPacketSize >= fullPacket.Length)
{
extraData = false;
RoomserverReceivedPackets.Enqueue(fullPacket.ToArray());
_queueNotifier.Set();
fullPacket.Close();
}
}
}
}
}
catch (SocketException soe)
{
logger.Error("_ReceivePackets " + soe.ToString());
}
catch (Exception ex)
{
logger.Error("_ReceivePackets " + ex.ToString());
}
}
}
private void _Response()
{
while (_socket.Connected)
{
_queueNotifier.WaitOne();
while (!RoomserverReceivedPackets.IsEmpty)
{
byte[] data = null;
if (RoomserverReceivedPackets.TryDequeue(out data))
{
MemoryStream bufferReceived = new MemoryStream(data, 0, data.Length);
using (var reader = new BinaryReader(bufferReceived))
{
int packetSize = (int)reader.ReadInt32();
byte reply = reader.ReadByte();
switch (reply)
{
case 0x01: // Login request
break;
case 0x02: // Login accepted
break;
case 0x03: // Enter room
break;
case 0x04: // Members list
break;
case 0x05: // Send Chat
break;
case 0x06: // Receive Chat
break;
case 0x07: // Receive Announcement
break;
case 0x08: // Send Announcement
break;
case 0x09: // Wrong password errors
_status = Status.RoomError;
break;
case 0x10: // Send Whisper
break;
case 0x11: // Receive Whisper
break;
case 0x12: // Leave Room
break;
case 0x13: // Disconnect
break;
}
}
}
}
}
}
}
}
On another classes I have:
public class SendPackets
{
public byte[] Data { get; set; }
public bool Delay { get; set; }
}
public enum Status
{
Disconnected = 0,
Connect,
EnterRequest,
RoomError,
Connected
}
- Use a
Dictionary<int, ICommandHandler>instead of your switch statement - Switch to asynchronous sockets
- Read up on Single Responsibility Principle.
- Use .NET naming convention
Then come back and ask a more specific question if you want a more specific answer.
Update to answer the comment
Instead of:
switch (reply)
{
case 0x01: // Login request
break;
case 0x02: // Login accepted
do:
public interface ICommandHandler
{
void Handle(byte[] packet);
}
public class LoginHandler : ICommandHandler
{
public void Handle(byte[] packet)
{
// handle packet data here
}
}
var myHandler = new LoginHandler();
myServer.Register(1, myHandler);
And then in your socket class:
public class MyServer
{
Dictionary<int, ICommandHandler> _handlers;
public void Register(int functionId, ICommandHandler handler)
{
_handlers[functionId] = handler;
}
private void _Response()
{
// .. alot of stuff ..
_handlers[reply].Handle(memoryStream);
}
Note that the example is far from complete and you might want to send in a context class instead of just the memory stream.
How far should I rely myself into the _socket.Connected ?
The Connected property gives you information as to the status of the socket at the last operation, so if the socket changes state since you last tried to read or write then Connected will give you the wrong (old) state.
As per the documentation, you should make a zero length send to get .NET to update the socket status. The success of this Send operation will tell you whether the socket is still connected.
Typically, only one thread is needed for communication in an application such as this. If all your application does is chat, then the entire thing could be single threaded. It would block your console if there is a read/write operation, but you could get around that by doing async read/write calls or by putting timeouts on blocking operations. You got a little overzealous with the threading in my opinion. Advice I give to new programmers is If you aren't sure whether you need multiple threads, start with a single threaded approach, and when you find areas where there is blocking or areas where performance could be improved with multithreading, then switch. Don't do it up front.
I see you use ReceiveFrom, which is intended for connectionless protocols. Try using basic receive instead. You should specify the number of bytes you want to receive, otherwise you risk overflowing the receive buffer. In C# this manifests itself with SocketException, and you would have to go dig through WinSock 2 API to figure out what the error code is. Much better to just specify a max size to receive and put the receive in a loop.
And I will echo what another responder has said - use single responsibility principle. Design a class that has one job only. For your design I would start with a class that encapsulates socket communications at a higher level for your application. Then I would derive that class into a server class and maybe a client class. Then you could use those classes in your "RoomServer" and "RoomClient" classes. This separation of concerns should force you to model each object as real world objects - talkers, and listeners, it makes you think about what each one needs, and extraneous member variables unrelated to that class's primary job need to be removed from the class and find a better home.
![Interactive visualization of a graph in python [closed]](https://www.devze.com/res/2023/04-10/09/92d32fe8c0d22fb96bd6f6e8b7d1f457.gif)
加载中,请稍侯......
精彩评论