Run a TCP server listening for incoming messages

I've written the following code, but I need some review on design style. I want to develop a library/module and expose an API to client programs. How do I do that?

The code just runs a TCP server listening for incoming messages and another thread accepting user input to send messages.

```
from socket import AF_INET, SOCK_DGRAM
import sys;
import socket;
import select;
import thread;
import pickle;
import struct;
import time;
import threading;

class AgTime:
lClock=0;
pClock=0;
count=0;

#NTP client object that queries NTP server for time.
class Ntp:

def __init__(self):
# # Set the socket parameters
self.host = "pool.ntp.org"
self.port = 123
self.buf = 1024
self.address = (self.host,self.port)
self.msg = '\x1b'+47*'\0'
# reference time (in seconds since 1900-01-01 00:00:00)
self.TIME1970 = 2208988800 # 1970-01-01 00:00:00

def time(self):
# connect to server
client = socket.socket( AF_INET, SOCK_DGRAM)
client.sendto(self.msg, self.address)
data, address = client.recvfrom( self.buf )

retval=struct.unpack("!12I",data);
t = retval[10]
t -= self.TIME1970
return t;

class server(threading.Thread):
def __init__(self,portNo):
threading.Thread.__init__(self);
self.PORTNO=portNo;
self.server_socket=socket.socket(socket.AF_INET,socket.SOCK_STREAM);
self.server_socket.bind(("",self.PORTNO));
self.CONNECTION_LIST=[];
self.RECV_BUFFER=4096;

def run(self):
self.server_socket.listen(10);
self.CONNECTION_LIST.append(self.server_socket);
print "Server started on port :"+str(self.PORTNO);

while(1):
read_sockets,write_sockets,error_sockets=select.select(self.CONNECTION_LIST,[],[]);
for sock in read_sockets:
if sock==self.server_socket:
sockfd, addr= self.server_socket.accept();

I'm just going to review the Ntp class. As you'll see, there's plenty here for one answer.

-
There's no documentation. What is this class supposed to do? What value does the time method return?

-
There are some "tells" that you have come from a C background, and are not yet a fluent Python programmer: you have unnecessary semi-colons at the ends of lines, and unnecessary parentheses around the condition in if statements.

-
The code is not portable to Python 3:

>>> Ntp().time()
 Traceback (most recent call last):
   File "", line 1, in 
   File "cr41537.py", line 20, in time
     client.sendto(self.msg, self.address)
 TypeError: 'str' does not support the buffer interface

For portability to Python 3 you need msg to be a bytes object, not a str.

-
Ntp is a "classic" or "old-style" class. This means that it doesn't support the super function, making it inconvenient to subclass. You should make new-style classes unless you have a good reason not to, by inheriting from object:

class Ntp(object):

-
It's not clear that there's any need for a class here. There's no persistent state, and just one method, so a function would work just as well and be simpler.

-
In this line:

self.msg = '\x1b'+47*'\0'

it is not at all clear where the expression comes from. What does it mean, and how can I check that you got it right?

By reference to the NTP standard in RFC 5905, it looks as though this decodes as version = 3 and mode = client. I would try to be much more explicit here as to how this packet is constructed, for example like this:

# See section 7 of RFC 5905 for the structure of an NTP packet.
    fmt = '!BBbb3I4Q'
    version = 3
    mode = 3 # client mode
    msg = struct.pack(fmt, (version << 3) + mode, *[0]*10)

Note that msg is now a bytes object as required by Python 3.

-
You call socket.recvfrom to get the address of the socket sending the data, but then you never use it. So why not just use socket.recv?

-
You are only going to use the first 48 bytes of the returned packet, so there is no point in setting the bufsize argument to socket.recv to be any larger.

-
You decode the result like this:

retval=struct.unpack("!12I",data);
 t = retval[10]

but looking at RFC 5905, this gets you the top 32 bits of the 64-bit transmit timestamp (the integer part). Why are you throwing away the bottom 32 bits? Python's datetime module supports computation with microseconds, so it is worth having both. I would decode like it this:

fmt = '!BBbb3I4Q'
    results = struct.unpack(fmt, data)
    tx_timestamp = results[10]

and then if I need to convert to seconds I can divide by 232. Note also that I used the same format for decoding the received packet as I did for encoding the sent packet. This ensures consistency.

-
In this code:

# reference time (in seconds since 1900-01-01 00:00:00)
 TIME1970 = 2208988800 # 1970-01-01 00:00:00

it is not clear where this value comes from. How would I check that you got it right? I would refer to figure 4 of RFC 5905 in the comment.

You could also consider computing this constant explicitly using Python's datetime module, which would make its meaning clear:

from datetime import datetime

    # Offset between NTP time and Unix epoch time in seconds.
    # See figure 4 of RFC 5905.
    TIME1970 = (datetime(1970, 1, 1) - datetime(1900, 1, 1)).total_seconds()

-
There is no attempt to correct for the transmission delay as described in section 8 of RFC 5905. You need something like this:

import time

 # Make sure DNS lookup time is not included in delay calculation.
 address = socket.getaddrinfo(host, port, AF_INET, SOCK_DGRAM)

 orig_time = time.time()
 client.sendto(msg, address[0][4])
 data = client.recv(48)
 dst_time = time.time()
 results = struct.unpack(fmt, data)
 recv_time = results[9] / 2.0**32
 tx_time = results[10] / 2.0**32

 # Estimate transmission delay: see section 8 of RFC 5905.
 delay = ((dst_time - orig_time) - (tx_time - recv_time)) / 2

Note that the purpose of calling socket.getaddrinfo here (instead of passing (host, port) directly to client.sendto) is to avoid the time for the DNS lookup of host being included in the delay calculation.

-
The time method returns the result in an inconvenient format (as the number of seconds since the epoch). It would be more generally useful to Python programmers to return a datetime.datetime object, like this:

from datetime import datetime

 return datetime.utcfromtimestamp(tx_time - TIME1970 + delay)

StackExchange Code Review Q#41537, answer score: 3