TCP chat room in Python3

Question

(Previous question)

I've created a server file, that keeps waiting for clients. When someone connects to the server, he can send messages to the server, and from the server it is broadcasted to all clients.

Can you criticize my simple chat? Any possible improvements and changes are welcome. I have 2 classes and 2 objects files:

Server class:

import threading
import socket
class Server:
 def __init__(self):
 self.host = str(socket.gethostbyname(socket.gethostname()))
 self.port = 5050
 self.server = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
 self.server.bind((self.host,self.port))
 self.server.listen(6)
 self.text_coding = "utf-8" #encoding and decoding format
 self.text_length = 30 #max bytes of the messages
 self.clients = [] #stores each client reference
 self.names = [] #stores clients names?
 self.disconnect = str("$hutdown") #safe word to close the connection and leave the chat
 #This function sends everyone message to everyone (including the one who send)
 def broadcast(self,msg):
 for i in self.clients:
 i.send(msg.encode(self.text_coding))
 #This function removes current client name and reference from the lists above
 #And warns the server and all clients that the current client left 
 def closing_client(self,client,name):
 self.clients.remove(client)
 self.names.remove(name)
 self.broadcast(f"{name} left the chat.")
 print(f"{name} left the chat.")
 #Each client has his own thread of this function
 #This function takes each client message, to send to broadcast().
 def handle_client(self,client_reference,client_info,name):
 print(f"Connected with {name}, {client_info}")
 
 while True:
 try:
 message0 = client_reference.recv(self.text_length).decode(self.text_coding)
 message = str(message0)
 #when someone uses the key word to leave the chat
 if message == str(f"{name}: {self.disconnect}"):
 self.closing_client(client_reference,name)
 break
 else:
 self.broadcast(message)
 except Exception as error:
 print("=================")
 print(error.__cause__)
 print("=================")
 self.closing_client(client_reference,name)
 break
 client_reference.close()
#This functions waits for new client
#And creates threads(of handle_client function) for each client
 def wait_new_client(self):
 while True:
 client_reference,client_info = self.server.accept()
 name = client_reference.recv(self.text_length).decode(self.text_coding)
 self.names.append(name)
 self.clients.append(client_reference)
 
 client_reference.send(str(f"Welcome {name}.").encode(self.text_coding))
 self.broadcast(str(f"{name} joined the chat."))
 
 thread = threading.Thread(target=self.handle_client, args=[client_reference,client_info,name])
 thread.start()

Server file:

from Server import Server
print("Starting server...")
server0 = Server()
print("=====================")
print(f"Server is listening at {server0.host}")
server0.wait_new_client()

Client class:

import socket
import threading
class Client:
 def __init__(self,name):
 self.name = name
 self.server_ip = "192.168.1.6"
 self.server_port = 5050
 self.text_lenght = 30
 self.disconnect = str("$hutdown")
 self.text_coding = "utf-8"
 self.state = True
 self.client = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
 self.client.connect((self.server_ip,self.server_port))
 self.client.send(self.name.encode(self.text_coding))
 #This function waits for chat messages
 def chat_messages(self):
 while self.state:
 try:
 message = self.client.recv(self.text_lenght).decode(self.text_coding)
 print(message)
 except Exception as error:
 print("=================")
 print(error.__cause__)
 print("=================")
 self.client.close
 self.state = False
 #This function waits for user input, new message
 def write(self):
 while self.state:
 try:
 text = str(input("Digit a message: "))
 message = str(f"{self.name}: {text}")
 self.client.send(message.encode(self.text_coding))
 if text == self.disconnect:
 self.state = False
 except Exception as error:
 print("=================")
 print(error.__cause__)
 print("=================")
 self.client.close
 self.state = False
 #This function put the above functions on threads.
 def start(self):
 try:
 chat_messages_thread = threading.Thread(target=self.chat_messages)
 write_thread = threading.Thread(target=self.write)
 chat_messages_thread.start()
 write_thread.start()
 except Exception as error:
 print("=================")
 print(error.__cause__)
 print("=================")
 self.client.close
 self.state = False

Client file:

from Client import Client
name = str(input("Type your name: "))
client0 = Client(name)
client0.start()

Answer 1

There are a few things I'd like to point out

The Server class

• It would be nice to be able to tell the server to listen on a different port, presumably by making the Server class' constructor take the port number as a parameter
• When you have a pair of lists that need to be kept in sync, it's often better to have a list of pairs instead. But in this case, clients presumably have unique names, so it might be nice to enforce that by having self.clients be a dict mapping names to client references
• In handle_client, you seem to be overusing the str function. It doesn't do any harm, but message0 should already be a string after decoding so message = str(message0) is unnecessary, and in the if statement the string literal is definitely a string so you can just do f"..." instead of str(f"...")
• except Exception is a bit smelly. Like, true, you do want to make sure that everything gets cleaned up properly if something unexpected happens, but usually you have some idea of what might go wrong, and maybe some way to recover from some of those
• closing_client is a bit weird -- it removes the client from the list of known client sockets, but it doesn't take any steps to make sure it's closed. That seems like it could easily lead to a resource leak -- is there a reason that method doesn't also close the socket?

The client class

• I feel like the constructor should definitely take the server IP and server port as parameters. Someone might want to connect to a different server
• self.state is a bit of a weird name -- it seems to be True when connected to a server and False otherwise, so maybe it could be called connected instead?
• None of the catch blocks actually close the socket -- they just mention the close function, but they don't call it
• text_length is misspelled as text_lenght

The protocol in general

• There's a bit of an awkward problem where a message containing the text "$hutdown" it literally the same as a $hutdown command. This isn't that big a problem when $hutdown is the only thing that isn't a literal message, but maybe you want to add more commands, maybe even some that are more complex. It could be useful to explicitly distinguish messages so you know that you know what everything's supposed to be
• It feels a bit weird that clients send their own name with each message -- a confused client might find itself unable to properly $hutdown, and a malicious client might try to send a rude message with someone else's name on it (and I don't think this server would notice if someone did -- it only checks names when processing $hutdown commands, right?). The server already knows who's sending the message, so this doesn't seem necessary -- it knows what socket belongs to whom, so if "Taylor"'s socket sends "Hello" the server can figure out that it should broadcast "Taylor: Hello"
• It doesn't seem like anyone actually checks that messages are within the acceptable length. Now, most of the time any data received just gets passed straight through to a different socket or stdout, so it usually isn't a problem. But what if someone's name is MyVeryVeryVeryLongNameIsCool? That's short enough to be accepted as a name, but long enough that MyVeryVeryVeryLongNameIsCool: $hutdown can't fit in a single message, so they can't disconnect properly. Or what if someone sends a message like Sam: It's pronounced with a /ŋ/? The whole message is valid as UTF-8 text, but if you try to decode that in 30-byte chunks you break the ŋ in half and get a UnicodeDecodeError

• irtexas19: I've been trying to understand your answer. In most part is very clear, but : "Or what if someone sends a message like Sam: It's pronounced with a /ŋ/ ? The whole message is valid as UTF-8 text, but if you try to decode that in 30-byte chunks you break the ŋ in half and get a UnicodeDecodeError" What does it mean?
• Sara J: @irtexas19 A byte can only have 256 different values, and there are far more than 256 different characters, so most characters are made up of multiple bytes. For example, when you do "😃".encode("utf8") you get four bytes, not just one. If I send you 60 bytes of text and you try to handle them 30 bytes at a time, it's possible that the 30th byte is in the middle of a multi-byte character, and you can't decode just part of a character, so even if my message was valid UTF-8 text when read as a whole, the first 30 bytes on their own might not be
• irtexas19: So should I make sure that everyone sends exactly 30bytes of message, before encoding it?
• Sara J: @irtexas19 That's an option - if the client expects 30-byte messages, then that's what the client should send. Though another option could be to have messages that include their own lengths, or have some sort of markers that show how long messages are, or where they begin and end, or something like that, so clients and servers can be sure to process complete messages