3
\$\begingroup\$

I am starting to learn network programming. I have implemented a very simple chat server based upon TCP, where the users only need to provide username. The assumption is that the server makes use of no additional threads and the sockets are not blocking. The code:


import sys
import time
import socket
import select


class User:
    def __init__(self, name, sock):
        self.name = name
        self.sock = sock


class Server:
    def __init__(self, host, port):
        self.users = []

        self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        self.sock.bind((host, port))

    def accept_usr(self):
        usock, _ = self.sock.accept()
        usock.send(b"Please, choose your username: ")
        uname = usock.recv(1024)
        self.users.append(User(uname, usock))
        _, writes, _ = select.select([], self._usocks(), [])
        for w in writes:
            w.send(bytes(f"{uname} has joined us!", encoding="ascii"))

    def handlerr(self, errs):
        for s in errs:
            s.close()

    def update(self):
        reads, writes, errs = select.select([*self._usocks(), self.sock], self._usocks(), self._usocks())
        for r in reads:
            if r is self.sock:
                self.accept_usr()
            else:
                uname = [u for u in self.users if u.sock is u.sock][0].name
                msg = r.recv(1024)
                for w in writes:
                    w.send(bytes(f"{uname}: {msg}", encoding="ascii"))
        self.handlerr(errs)

    def run(self):
        self.sock.listen(5)
        while 1:
            self.update()
            time.sleep(0.1)

    def _usocks(self):
        return [u.sock for u in self.users]

if __name__ == "__main__":
    s = Server("localhost", int(sys.argv[1]))
    s.run()

I would be grateful for any hints and comments.

EDIT: An obvious improvement that comes to my mind is to store the mapping of socket->User in a dict so that I'll be able to quickly determine the author of the message being sent.

\$\endgroup\$
2
\$\begingroup\$

Packet fragmentation

Have a read through this: https://stackoverflow.com/a/17668009/313768

usock.recv(1024) is fragile, and under many circumstances simply won't work for a handful of reasons. You'll need to do some reading about the fundamentals of TCP/IP to understand why that is, but basically, you can't assume that you'll get the data you want all in one chunk.

Context manager

You hold onto some sockets, which are resources that should be closed when you're done with them even if something goes wrong. This is a job for a context manager. You can make your class a context manager so that calling code can be sure that your resources are closed off, regardless of what else happens.

Have a read through this: https://docs.python.org/3/library/stdtypes.html#typecontextmanager

Presentation versus logic

    usock.send(b"Please, choose your username: ")

Typically, this is not done, and instead a pre-chosen number is sent over the wire, leaving the client to choose how to ask the user a particular thing. This becomes especially important when graphics get involved, or when you need to do translation/internationalization.

The first thing

[u for u in self.users if u.sock is u.sock][0].name

should be

next(u for u in self.users if u.sock is u.sock).name

That said... what on earth is this doing? When would that condition ever evaluate to False?

Type hints

PEP484 type hints, such as host: str, port: int, will clarify your function arguments.

\$\endgroup\$

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.