Hot answers tagged

7

These are good questions! 1. You should indeed have the copy constructor & assignment operator deleted. The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-...


7

Portability The code uses pthread which is a POSIX (Portable Operating System Interface) structure. If the code uses POSIX for one thing, it might be better to use POSIX for all things to remain POSIX compliant. This means that instead of using bzero((char*)&serv_add, sizeof(struct sockaddr_in)); to zero out the serv_add variable it might be better use ...


6

There's one major problem with the code: Your move assignment is broken. You forgot to close(_s) before overwriting it with other._s. To avoid this kind of problems, I suggest using the copy-and-swap idiom. It makes writing a operator= a no-brainer in most cases: tcp_socket &operator=(tcp_socket other) noexcept // Note the lack of `&&`. { ...


5

Review Property Connected is not thread-safe. Use Interlocked.CompareExchange to get the connection atomically. Events DataReceived and Disconnected are never cleared. This will cause a memory leak. Implement a Clear function and/or implement IDisposable. Methods Send, Disconnect, EndReceive are not thread-safe. Consider using a mutex/lock. Checking ...


4

As other answers have said, use swap to get assignment correct, and don't close already-closed descriptors. Some minor style nitpicks to add: Use initializers in preference to assignment in the constructors. There's no need to name the arguments to the deleted copy methods. Remove the redundant parens from return *this;. Consider a public close() method, ...


4

regarding: if(n<0){ printf("Buffer: %s", buffer); error("Reading Failed"); } the call to printf() will change the value in errno. The result is the call to perror() in function: error() will print the wrong message. regarding: void* job_read(void * p){ int* socketp = (int*)p; int newsockfd = (*socketp); the variable ...


4

Idiomatic Rust Standard utilities Much of your code is contained in the std function std::io::copy, which you can use to avoid handling each side of the pipe: match io::copy(&mut reader, &mut writer) { Ok(0) => return (), Err(e) if e.kind() != io::ErrorKind::WouldBlock => return (), _ => () } And you can also use a for loop ...


3

Instead of always manually parsing sys.argv, you can use the standard library tool argparse. It automatically enforces the right usage and prints a nicely formatted usage if not. For your given example it would look like this: import argparse def ip_address(ip): # TODO: implement checking if IP is valid return ip def parse_args(args=None): ...


3

Here are some things that may help you improve your code. Eliminate unused variables The parameter hb in the print() function and value in main() are unused variables and should be omitted from the program. Fix your formatting There are inconsistent spaces at the beginning of lines and inconsistent indentation. Being consistent helps others read and ...


2

You shouldn't store the cancellationToken. If someone was to call AcceptTcpClientAsync twice it would overwrite it. Also it starts making your async call keep state and that's not something you want to do or even need to do in this case. Also need to wrap the Register into a using statement such as // Register a Callback to be executed when the token is ...


2

pthread_create returns value which is needed to be checked too; if your system runs out of resources, the new thread might not start. You are listening on all IPs available; considering that there is no visible authentication, this opens possibility for unauthorized access, impersonation etc. Consider listening on localhost only (enough within one server); ...


2

I read through your profile and saw you are a 17 year old developer hobbiest. I didn't know what OO was at that age. Well done. I made this review under the impression you were a somewhat experienced professional. AbstractSocketWrapper The constructor silently catches IOException. This leaves the socket in a corrupted state. Throw any exception up the ...


2

This looks very reasonable, but some improvements are possible: getsockname() returns the local side's address In your code you write: client_sock, client_addr = sock.accept() ... print('Connection from', client_sock.getsockname()) That should have been client_sock.getpeername(). But you don't need to call any extra function, since the address of the ...


2

Specification You have 2 requirements stated below. I see no reason why to re-invent-the-wheel here. Both requirements can be met with using common practices. ..or a mechanism to drop packages rather fast before reading all the package content. (In case somebody sends an array with 10k elements, continuously to make denial of service). This should ...


2

Few things, Your properties should be all set to readonly as you need to enforce your class requirements. You can make them public, but with a private set;. or simply { get; }. TcpClient is a main requirement in this class, and you didn't setup the class constructor correctly for that purpose. Because of that, you were constructing inside Connect(). For ...


1

Prefer static const variables over #defines Don't #define constants if you could just as well declare them as regular static const variables. One advantage of this is that you can put those variables inside a namespace, whereas macros are always globally visible. Avoid giving new names to existing types Don't make a new type Byte when it's just the same ...


1

Final version of the class, incorporating changes suggested by CharlesNRice, and removing the unnecessary callback method. public class TcpListenerEx : TcpListener { #pragma warning disable 618 [Obsolete("This method has been deprecated. Please use TcpListenerEx(IPAddress localaddr, int port) instead. https://go.microsoft.com/fwlink/?linkid=14202")] ...


1

You need to use some kind wire protocol. In simplest case a triple (int length, int/enum for tag/type, byte[] data). Then you will be able to differentiate between command "ping" and a sending a file with content "ping". Also look into ProtoBuf or ObjectInputStream - it is futile to try parsing objects from toString() output.


1

There is a lot of code duplication between job_read and job_write, and all the work is being done from a lot of threads inside a big mutex that will only allow one thread ever to actually do something. Servicing all clients from a single thread using select(2) would be more efficient. The most annoying failure mode for this setup is that if one of the ...


1

Events can be cleared much simpler: public void Clear() { foreach (Delegate d in DataReceived.GetInvocationList()) DataReceived -= (EventHandler<NetworkReceiveEventArgs>)d; foreach (Delegate d in Disconnected.GetInvocationList()) Disconnected -= (EventHandler)d; } public void Clear() { DataReceived = null; Disconnected ...


1

Docstrings You should include a docstring at the beginning of every method, class, and module you write. This will allow documentation to identify what your code is supposed to do. It also helps external text editors, such as VSCode, to display what types of parameters your functions accept, and a short description of your method. Error or Error This line ...


1

This is a nice snippet which makes it useful for teaching! Here are some points: Make imports explicit Though import socket, threading is valid in Python, importing in two lines improves readability import socket import threading Two lines after imports Add two lines after imports. From this: import socket import threading host = socket.gethostname() .....


1

ClientController I don't see a purpose for this class. I would move AllClients, AddNewClient, RemoveClient to Listener and AddClientRequest to Client. These operations should be made thread-safe. Client Don't start an async operation int the constructor. Create a method Initialise() and let this method call Receive.StartReceiving(). ClientReceiver ...


1

The methods DecodeInt32 and DecodeInt16 are very confusing. The bitwise or (|) of 2 bytes can never give you something less than 0. Furthermore, bytes[0] << 24 can give you a negative number. I really have no idea what these methods are meant to achieve. Also, your packetId seems to be included in the packetLength, which may create issues in the ...


Only top voted, non community-wiki answers of a minimum length are eligible