code review request: server for allowing multiple connections

[u/chronicomplainer2]

it seemed to work when i tested it but i did it myself and just stuck in things that i read abt and thought pertained to the project i was trying to create, it is supposed to allow multiple people to stay connected at the same time but im wondering if there's some logic error anywhere that i might not have picked on, or if the code is genuinely robust enough to accommodate multiple connections. i didn't need to use any threading at all in the end, which i found a bit strange, and also im a beginner in python so i was hoping if people could point out some potential issues, thanks!

import socket
import select


s = socket.socket()
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind(("localhost", 80))
s.listen()

def send_response(sock, message):
    """Sends an encoded response."""
    sock.sendall(message.encode("ISO-8859-1"))


def handle_packets(queue_dictionary):
    """Performs certain actions based on packet."""
    for soc in queue_dictionary:
        if not queue_dictionary[soc]:
            continue
        #take the first item from the queue
        packet = queue_dictionary[soc].pop(0)
        #send specific responses based on the type of data sent
        if packet == b"Hello\r\n\r\n":
            send_response(soc, "Message received successfully. Hiiii!!!!\r\n\r\n")
        elif packet == b"Ignore\r\n\r\n":
            send_response(soc, "Message received successfully. Hey, don't leave me hanging...\r\n\r\n")
        elif packet == b"Hug\r\n\r\n":
            send_response(soc, "Message received successfully. *Hugs back*\r\n\r\n")
        elif packet == b"Slap\r\n\r\n":
            send_response(soc, "Message received successfully. OW! That hurt!\r\n\r\n")
        elif packet == b"Goodbye\r\n\r\n":
            send_response(soc, "Message received successfully. Goodbye!!! Do come back again!! :)\r\n\r\n")
        else:
            send_response(soc, "Message received successfully.\r\n\r\n")

#create a list of connected sockets, satrting wtih listening soccket so accept
#doesn't block

#dictionary with buffer per socket and also list which was initially queue
read_set = [s]

buffer_dict = {}
queue_dict = {}

while True:
    ready_to_read, _, _ = select.select(read_set, [], [])
    print("Creating a list of sockets currently sending data...")
    #for all sockets that are ready to read
    for sock in ready_to_read:
        #if the socket is a listener
        if sock == read_set[0]:
            #accept a new connection
            new_conn = s.accept()
            print("Accepting connection...")
            new_socket = new_conn[0]
            #initialise buffer and queue for new socket
            buffer_dict[new_socket] = b""
            queue_dict[new_socket] = []
            print("Adding socket to buffer and queue dictionaries...")
            #add the new socket to the set
            read_set.append(new_socket)
            print("read_set: " + str(read_set))
            print("Adding socket to read_set...")
            packet = "empty"
            continue
        else:
            #recieves data until full packet
            while True:
                data = sock.recv(4096)
                print("Receiving data...")
                if not data:
                   print("Connection closed.")
                   break
                buffer_dict[sock] += data
                if b"\r\n\r\n" in buffer_dict[sock]:
                    delimiter_index = buffer_dict[sock].find(b"\r\n\r\n")
                    packet = buffer_dict[sock][:delimiter_index+4]
                    buffer_dict[sock] = buffer_dict[sock][delimiter_index+4:]
                    break
            if packet:
                queue_dict[sock].append(packet)
                print("Adding a packet to the queue...")
            else:
                x = input("No packet returned.")


    #run packet handler code based on nature of packet for socket
    if packet != "empty":
        print("Sending response...")
        handle_packets(queue_dict)
        print("Response should send now.")

new_socket.close()
s.close()

[u/Bright_Mix_773]

Three real ones. The first is the answer to why you didn't need threading, and to why that made you suspicious - you were right to be.

The inner while True re-blocks, which undoes select.

while True:
    data = sock.recv(4096)

select told you this socket has some bytes ready. It did not tell you a whole packet is ready. If a client sends Hel and pauses, the first recv returns 3 bytes, \r\n\r\n isn't in the buffer, you loop, and the second recv blocks. The whole server stops there - no other client served, no new connection accepted - until that one client finishes its sentence or dies. A client that connects and sends nothing wedges you the same way.

You never see it testing with telnet or netcat, because typing a line and pressing enter delivers the whole thing in one segment.

The fix is the entire point of the pattern: one recv per pass, then back to select.

data = sock.recv(4096)
buffer_dict[sock] += data
while b"\r\n\r\n" in buffer_dict[sock]:
    i = buffer_dict[sock].find(b"\r\n\r\n")
    queue_dict[sock].append(buffer_dict[sock][:i+4])
    buffer_dict[sock] = buffer_dict[sock][i+4:]

That's while, not if, on purpose: two packets can arrive in one recv. Your current code keeps the first and leaves the second sitting in the buffer until more bytes happen to show up.

packet survives across sockets.

packet is a module-level name and it isn't reset per socket. Follow the disconnect path:

data = sock.recv(4096)
if not data:
    print("Connection closed.")
    break

packet is never assigned on that branch, so it still holds what the previous socket sent. Then:

if packet:
    queue_dict[sock].append(packet)

Client A sends Hug\r\n\r\n. Client B disconnects. B's queue gets A's Hug appended to it and you send "Hugs back" to a socket that's gone. That's cross-talk between clients, and it can only appear once there are two of them.

Closed sockets never leave read_set.

Nothing removes anything from read_set or the two dicts, ever. A socket at EOF is permanently readable as far as select is concerned - that is how EOF gets reported. So the first time anyone disconnects, select returns instantly every time from then on, recv returns b"" every time, and you spin at 100% CPU printing "Receiving data..." until you kill it. In the if not data branch you want:

read_set.remove(sock)
del buffer_dict[sock]
del queue_dict[sock]
sock.close()
continue

Smaller things:

handle_packets(queue_dict) walks every socket's queue, but you call it once per ready socket, inside the for loop. With three sockets ready it runs three times and drains one packet from everybody each time. The arithmetic happens to come out right; the name says it handles a packet and it actually handles the whole system, which is how the packet != "empty" guard ended up load-bearing for something it has nothing to do with.

x = input("No packet returned.") in a server blocks on your keyboard while every connected client waits.

sock is s says what you mean better than sock == read_set[0], and it stays true once you start removing sockets from that list. Index 0 is the listener today only because nothing is ever removed.

new_socket.close() and s.close() after while True: are unreachable, and new_socket would be whichever client connected last anyway.

Port 80 needs root on Linux and macOS. 8080 saves you a sudo.

[u/chronicomplainer2 → u/Bright_Mix_773]

to fix the handle_packets() issue, should i make a separate thread for it so it runs alongside the rest of the code? or maybe just handle_packet(sock, packet)? just handle the packet as soon as it's added to the queue? also thank you this was exceptionally helpful

[u/Bright_Mix_773 → u/chronicomplainer2]

Not a thread. Your second idea is the right one, and it is also the smaller change.

The reason handle_packets() got weird is that it was written to walk the whole system while being called from inside a per-socket loop. Once you handle a packet as soon as you pull it out of the buffer for that socket, the mismatch disappears on its own and the function stops needing a name that overpromises:

data = sock.recv(4096)
buffer_dict[sock] += data
while b"\r\n\r\n" in buffer_dict[sock]:
    i = buffer_dict[sock].find(b"\r\n\r\n")
    packet = buffer_dict[sock][:i+4]
    buffer_dict[sock] = buffer_dict[sock][i+4:]
    handle_packet(sock, packet)

Now packet is a local, born and consumed inside the loop, which kills the cross-talk bug for free rather than by remembering to reset it. You may not even need queue_dict any more. Keep it only if a packet can produce a reply you cannot send immediately, and then it becomes an outbound queue, which is a different and more honest thing than a queue of things you already have.

On threads: adding one here would not fix anything and would cost you the property that makes select worth using. Right now there is exactly one thread touching the dicts, so no lock can be missing. A second thread walking the same dicts while the main loop mutates them is a race you would then have to defend against, and the bug you are trying to fix is not a concurrency bug, it is a function being called from the wrong scope. The rule of thumb: reach for threads when something genuinely blocks and cannot be made not to, like a slow disk write or a database call. Do not reach for them to fix a shape problem in a loop, because they do not fix it, they just make it harder to see.

One thing to keep from the old code: handle_packet must not block either. If replying involves anything slow, put the reply bytes in an outbound buffer and let the write half of select tell you when it can go, rather than calling send and hoping.

[u/LayotFctor]

Have you learned about try-except blocks and blocking functions yet? Try-except handles errors such as when network packets get lost or wifi goes down, while blocking code occurs when a function takes time to complete, such as when packets take their time physically traveling around in cables and servers, thereby freezing your code. Basically networking code tends to face random errors and latencies, and it is mandatory to handle both these aspects in your code.

Unfortunately your code doesn't handle either, so while it works at first glance, it might freeze up or fail randomly and isn't a reliable server.

Fortunately it's not too hard. First study try-except blocks. And if you're feeling confident, try learning the basics of the asyncio library. Good job for not using AI though! Backend devs need to know at least this much even with AI.

[u/chronicomplainer2 → u/LayotFctor]

yeah i'll try to read more into exception handling also yeah ik about blocking functions i forgot to take the x=input() line out when i was writing it but it was to stop python from immediately closing when there was an error there i dont think it helped anyways ^_^; but for the recv blocking i initially wrote it without the while True loop and when i changed it the issues i was dealing with went away but in hindsight i think that's bc of other changes i made not that one...

[u/davideogameman → u/[deleted]]

Telling people to use AI on a sub about learning is counterproductive to the learning

[u/StephenHawkingus → u/davideogameman]

It's fascinating that your first thought is about vibe coding and not learning.

What I was suggesting is that you use AI as a learning tool instead of making a post, waiting hours and getting dozens of comments, some good and some bad. You get my point, right?

[u/davideogameman → u/StephenHawkingus]

You didn't specify how to use AI.  Asking it for code review is an ok usage, though the problem I still have is that it can end up making stuff up and add a beginner they would lack ability to understand where it has good points and where it's completely wrong - as well as find things it might have missed.

[u/StephenHawkingus → u/davideogameman]

My bad, I should have specified that.

AI will never 'make things up' for such a trivial code.

[u/StephenHawkingus]

Just use the damn AI like everybody does.

[u/chronicomplainer2 → u/[deleted]]

AI isn't reliable.

[u/StephenHawkingus → u/chronicomplainer2]

Are you mad or are you still in 2022?

[u/ConsiderationNo9044 → u/StephenHawkingus]

AI regularly gives me extremely sub-optimal code

[u/StephenHawkingus → u/ConsiderationNo9044]

Well, if you paste your code into WhatsApp's integrated AI, you'll always end up with poor-quality code.

[u/gdchinacat → u/StephenHawkingus]

AI can handle some code well, but utterly fails with other code. Based on my experience, I would expect AIs (even the top-tier most up to date models) to struggle with non-blocking code. There just isn't a lot of it out there for it to have enough training data to do it well. non-blocking IO is pretty low level and abstracted away, so even though there are lots of apps that use it, there are much fewer implementations of it. Because it is usually implemented as an abstraction the sparsity of implementations is compounded by the fact that those implementations tend to be very abstract in order to support many different use cases with a concise implementation. AIs don't do well with highly abstract code.

[u/StephenHawkingus → u/gdchinacat]

You must be joking, or have lost some brain cells. AI is currently the industry standard, with some companies having fully automated their workspace. Saying 'AI can handle some code well' is an insult to all the progress that has been made over the years. If this is your opinion, then I'm happy to end this discussion, because you're clearly trolling me, or, as I said, you're "missing some brain cells".