4
\$\begingroup\$

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 files:

Server:


import threading
import socket

text_type = "utf-8"
host = str(socket.gethostbyname(socket.gethostname()))
port = 5050

server = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
server.bind((host,port))
server.listen(4)

clients = []
names = []

#This function sends message to all clients on room
def broadcast(msg):
    for i in clients:
        message = msg.encode(text_type)
        i.send(message)

#This function  waits until someone send a message, and then pass it to broadcast()
def get_message(client_reference):
    while True:
        try:

            message = client_reference.recv(1024).decode(text_type)
            broadcast(message)

        except:

            index = clients.index(client_reference)
            clients.remove(client_reference)
            client_reference.close()
            name = names[index]
            broadcast(f"{name} left the chat.".encode(text_type))
            names.remove(name)
            break

#This function waits to new clients,  and then store their name and other informations
def new_client():
    while True:

        client_reference,client_info = server.accept()

        name = client_reference.recv(1024).decode(text_type)
        print(f"Connected with {name}, {client_info}")

        names.append(name)
        clients.append(client_reference)
        client_reference.send(str(f"Welcome {name}.").encode(text_type))

        broadcast(str(f"{name} joined the chat."))
       
        thread = threading.Thread(target=get_message, args=[client_reference])
        thread.start()


new_client()

Client:

import socket
import threading
text_type = "utf-8"

server_ip = "192.168.1.6"
port  = 5050

client = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
client.connect((server_ip,port))

name = str(input("Type your name: "))
client.send(name.encode('utf-8'))

#This function gets other clients messages, so any client has access to all messages
def messages():
    while True:
        try:
            message = client.recv(1024).decode(text_type)
            print(message)
        except:
            print("ERROR")
            client.close()
            break

#This function waits for the user input(message)
def write():
    while True:
        text = str(input("Digit a message: "))
        message = str(f"{name}: {text}")
        client.send(message.encode(text_type))


receive_thread = threading.Thread(target=messages)
receive_thread.start()

write_thread = threading.Thread(target=write)
write_thread.start()


```
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Jun 5, 2021 at 9:21

1 Answer 1

4
\$\begingroup\$
  • text_type is really an "encoding", not a type, so there is a better variable name than this
  • server should not be a global
  • server should be wrapped in a with - see here about the docs calling out support for context management, and here for general information on context management
  • Never try/except; you're going to want to catch a more specific exception
  • For something that passes around messages whose sequence is not critical, UDP will be more efficient than TCP. Take this with a grain of salt: UDP will also lose a guarantee of packet delivery, so packets may be dropped. Both out-of-sequence and dropped packets are typically rare, so it's a judgement call. UDP is frequently used for streamed audio.
  • recv(1024) is not a reliable way to get a whole message. Messages may be fragmented between the client and the server, and recv offers no guarantee of returning a whole message. Since your messages are plain strings, about the worst that could happen is that one message gets fragmented within a multi-byte unicode sequence. Read this, in particular starting at Now we come to the major stumbling block of sockets - send and recv.
  • clients.remove(client_reference) (and index) have O(n) worst-case performance. So a different data structure would help.
  • Your current broadcast, in addition to broadcasting to all other clients, broadcasts to the sending client itself. Are you sure that's what you intended?
  • Whenever you have two sequences, in this case clients and names whose elements logically align to represent "one thing" (a client), this is a clear case for a single sequence of class instances
  • Currently, your design for the client protocol basically accepts any string at all and broadcasts it with no validation or constraints to every other client. One very funny security vulnerability among many is that someone with a trivially-modified client could send anything for their name, and have it change on each message. It's trivially easy to impersonate other users in this system.
\$\endgroup\$
8
  • \$\begingroup\$ What should I use instead of recv()? \$\endgroup\$
    – irtexas19
    Jun 5, 2021 at 11:37
  • \$\begingroup\$ What does "server should be wrappen in a with" mean? These are the only points that I don't understand. \$\endgroup\$
    – irtexas19
    Jun 5, 2021 at 11:53
  • \$\begingroup\$ I would disagree with using UDP over TCP: Switching the order of two messages that are sent at almost the same time is fine, if weird, but losing packets is something I would consider unacceptable, especially since messages may be fragmented, so you would miss part of a message. \$\endgroup\$ Jun 5, 2021 at 13:34
  • \$\begingroup\$ "For something that passes around messages whose sequence is not critical, UDP will be more efficient than TCP" Yes, but I don't think that applies here. Language depends on context, so re-ordering a person's messages can affect the meaning and cause misunderstandings. And even if that wasn't an issue, you also appear to have control messages, and getting alice2 is now called alice and alice disconnected out of order is not ideal \$\endgroup\$
    – Sara J
    Jun 5, 2021 at 13:41
  • 1
    \$\begingroup\$ @irtexas19 re. recv - read docs.python.org/3/howto/sockets.html, in particular starting at Now we come to the major stumbling block of sockets - send and recv \$\endgroup\$
    – Reinderien
    Jun 5, 2021 at 14:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.