1
\$\begingroup\$

The below code is a simple TCP echo server. The folks using it claim "it is going down every now and again". Any thoughts on how this could be improved would be really appreciated, particularly if they would improve the reliability.

import select, argparse, socket

def server_socket(ip, port):
    serv = socket.socket()
    try:
        serv.bind((ip, port))
    except:
        if (ip != ''):
            print("Failed to bind to " + ip + " on port " + str(port) + " exiting" )
        else:
            print("Failed to bind to all on port " + str(port) + " exiting" )
        exit()

    if (ip != ''):
        print("Bound to " + ip + " on port " + str(port) + " echoing tcp" )
    else:
        print("Bound to all on port " + str(port) + " echoing tcp" )

    serv.listen(1)
    inputlist = [serv,]

    while 1:
        inputready,_,_ = select.select(inputlist,[],[], 20)

        if not inputready:
            for i in inputlist:
                if i != serv:
                    i.close()
                    inputlist.remove(i)
                    print("connection timeout")

        for i in inputready:
            if i == serv:
                client, address = serv.accept()
                inputlist.append(client)
                print("connection open from " + str(address))
            else:
                try:
                    data = i.recv(1024)
                    print("received " + str(len(data)) + " bytes"),
                    print(" \""+ data +"\"")
                    if data:
                        i.send(data)
                    else:
                        i.close()
                        inputlist.remove(i)
                        print("connection closed")
                except:
                    i.close()
                    inputlist.remove(i)
                    print("connection closed")


if __name__ == '__main__':
    parser = argparse.ArgumentParser(prog = "TCP Echo server")

    parser.add_argument('-i', '--ip', type=str, default='', help = "IP to bind to")
    parser.add_argument('port', type=int, help = "Port to bind to")

    args = parser.parse_args()
    server_socket(args.ip, args.port)
\$\endgroup\$
2
\$\begingroup\$

The folks using it claim "it is going down evey now and again".

Well, yes. If they don't send you a message in 20 seconds, you're disconnecting them! Why? Simply removing that block is probably what you want.

Separation of Concerns

Your server_socket function both creates a server and handles the busy loop. You should split those into two functions:

def make_server(ip, port):
    ...

def run_echo(server):
    ...

server = make_server(ip, port)
run_echo(server)

Echoing

You wrap your echo block in a try/except. Why? What part of that code can raise an exception? What exception is raised? You should prefer to only catch precisely the exceptions that are raised (to avoid, for instance, catching SyntaxError and hiding whatever typo you wrote in your code), but in this case I don't know what exception you would be catching.

Also, you don't need to print str(len(data)), you can just print len(data).

\$\endgroup\$
  • \$\begingroup\$ Thanks very much for your thoughts, Just to be clear, "going down" means no new connections can be made. The 20 second cutoff is by design, If the connection to a client is lost, will the socket remain open indefinitely without this block? You also mentioned the try/catch around the echo portion, I believe recv throws an exception when there is no data to be read, you are quite correct in that I should specify this particular exception. \$\endgroup\$ – jayjay Oct 23 '15 at 17:14

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.