10
\$\begingroup\$

I would like to ask about your opinion about my code. The idea is simple: I designed my own protocol, where client asks the server about the image, and the server sends the image, following the below steps (this is the actual protocol I wanted to implement):

CLIENT                                      SERVER
                      GET\r\n   
       ----------------------------------->
                      OK\r\n
       <----------------------------------- 
                   GET_SIZE\r\n
       ----------------------------------->
                    SIZE 1024\r\n
       <-----------------------------------
                   GET_IMG\r\n
       ----------------------------------->
    IMG_DATA with when image sending is over EOF\r\n
       <-----------------------------------

Everything works (it seems so) but I would like to ask about what else I could possibly improve here.

  1. Does the implementation reflects in 100% what I wanted to achieve in my protocol flow?
  2. Are the steps implemented as I wanted them to be in my protocol?

client.py:

#!/usr/bin/env python

import socket
import sys

HOST = '127.0.0.1'
PORT = 6666

sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
server_address = (HOST, PORT)
sock.connect(server_address)
fname = 'fromserver.png'

try:

    sock.sendall("GET\r\n")
    data = sock.recv(4096)

    if data:
        txt = data.strip()
        print '--%s--' % txt

        if txt == 'OK':

            sock.sendall("GET_SIZE\r\n")
            data = sock.recv(4096)

            if data:
                txt = data.strip()
                print '--%s--' % txt

                if txt.startswith('SIZE'):

                    tmp = txt.split()
                    size = int(tmp[1])

                    print '--%s--' % size

                    sock.sendall("GET_IMG\r\n")

                    myfile = open(fname, 'wb')

                    amount_received = 0
                    while amount_received < size:
                        data = sock.recv(4096)
                        if not data :
                            break
                        amount_received += len(data)
                        print amount_received

                        txt = data.strip('\r\n')

                        if 'EOF' in str(txt) :
                            print 'Image received successfully'
                            myfile.write(data)
                            myfile.close()
                        else :
                            myfile.write(data)
finally:
    sock.close()

server.py:

    #!/usr/bin/env python

import random
import socket, select
from time import gmtime, strftime

image = 'tux.png'

HOST = '127.0.0.1'
PORT = 6666

connected_clients_sockets = []

server_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)

server_socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
server_socket.bind((HOST, PORT))
server_socket.listen(10)

connected_clients_sockets.append(server_socket)

while True:

    read_sockets, write_sockets, error_sockets = select.select(connected_clients_sockets, [], [])

    for sock in read_sockets:

        if sock == server_socket:

            sockfd, client_address = server_socket.accept()
            connected_clients_sockets.append(sockfd)

        else:
            try:
                data = sock.recv(4096)

                if data :

                    txt = data.strip()
                    print '--%s--'%txt

                    if txt == 'GET' :
                        sock.sendall('OK\r\n')

                    elif txt == 'GET_SIZE' :

                        with open ('tux.png','rb') as f1:
                            file_size = len(f1.read())
                            f1.seek(0)

                        print '--%s--'%file_size

                        file_size = '%s' % file_size
                        sock.sendall('SIZE %s\r\n' % file_size)

                    elif txt == 'GET_IMG' :
                        with open(image, 'rb') as fp:
                            image_data = fp.read()

                        msg = '%sEOF\r\r' % image_data
                        sock.sendall(msg)
                        print msg

            except:
                sock.close()
                connected_clients_sockets.remove(sock)
                continue

server_socket.close()
\$\endgroup\$
3
  • \$\begingroup\$ I'm assuming you're using Python 2.7. If you're using Python 2.6 instead and are using features that are not available in 2.7 (I don't know all of them by heart), please replace the 2.7 tag by 2.6. \$\endgroup\$ – Mast Mar 2 '17 at 12:34
  • \$\begingroup\$ @Mast: I'm using Python 2.7.12 \$\endgroup\$ – yak Mar 2 '17 at 16:50
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Peilonrayz Mar 2 '17 at 16:55
2
\$\begingroup\$

One comment about the recv function: you should check that you indeed received the complete request/reply. For blocking sockets, recv will indeed block until some data is received, but it may return even after only 1 byte has been received. So for example in your client, you might receive something like "SIZE 1" as truncated version of "SIZE 1024\r\n". You'd have to keep recv'ing until you get to the "\r\n", for example by defining a "recvline" function that loops calling recv and appends the data into a result buffer, until "\r\n" is in the result (and perhaps stops after a maximum length is exceeded to avoid huge memory allocations if server has gone crazy :P).

Your commands are pretty short, so this is unlikely to happen unless there's some pretty bad connection issue, but still worth keeping in mind.

It is even possible that you might not detect that with the "EOF" mechanism that you implemented, because it is possible that by pure chance, the image data at the point of truncation contains the string "EOF". Then you'd have received "successfully" a corrupted image.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I tried to follow your advices and modified my code. I also changed EOF to EOIMG. Is it ok now? The only thing that bothers me now, is that when I run the server, and the client (image is copied successfully), I have 100% of CPU used by python. Is it normal? \$\endgroup\$ – yak Mar 2 '17 at 16:46

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.