Primitive Python P2P Socket Chat

Question

Here is my code for a Python script for a peer to peer chat system on a network using sockets. I know there are some things that could be changed so there would be less repetition in the code and that some variables could be global, but what I am asking is if there are any issues with the code's functionality; it seems to work properly on my home network but will only connect (not receive messages) on my school network. Because it establishes a connection there, it looks like the port is not being blocked by the internal firewall.

#!usr/bin/env python
import socket
import threading
import select
import time
import datetime
def main():
 class Chat_Server(threading.Thread):
 def __init__(self):
 threading.Thread.__init__(self)
 self.running = 1
 self.conn = None
 self.addr = None
 def run(self):
 HOST = ''
 PORT = 8080
 s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
 s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
 s.bind((HOST,PORT))
 s.listen(1)
 self.conn, self.addr = s.accept()
 # Select loop for listen
 while self.running == True:
 inputready,outputready,exceptready \
 = select.select ([self.conn],[self.conn],[])
 for input_item in inputready:
 # Handle sockets
 message = self.conn.recv(1024)
 if message:
 print "Daniel: " + message + ' (' + datetime.datetime.now().strftime('%H:%M:%S') + ')'
 else:
 break
 time.sleep(0)
 def kill(self):
 self.running = 0
 class Chat_Client(threading.Thread):
 def __init__(self):
 threading.Thread.__init__(self)
 self.host = None
 self.sock = None
 self.running = 1
 def run(self):
 PORT = 8080
 self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
 self.sock.connect((self.host, PORT))
 # Select loop for listen
 while self.running == True:
 inputready,outputready,exceptready \
 = select.select ([self.sock],[self.sock],[])
 for input_item in inputready:
 # Handle sockets
 message = self.sock.recv(1024)
 if message:
 print "Daniel: " + message + ' (' + datetime.datetime.now().strftime('%H:%M:%S') + ')'
 else:
 break
 time.sleep(0)
 def kill(self):
 self.running = 0
 class Text_Input(threading.Thread):
 def __init__(self):
 threading.Thread.__init__(self)
 self.running = 1
 def run(self):
 while self.running == True:
 text = raw_input('')
 try:
 chat_client.sock.sendall(text)
 except:
 Exception
 try:
 chat_server.conn.sendall(text)
 except:
 Exception
 time.sleep(0)
 def kill(self):
 self.running = 0
 # Prompt, object instantiation, and threads start here.
 ip_addr = raw_input('Type IP address or press enter: ')
 if ip_addr == '':
 chat_server = Chat_Server()
 chat_client = Chat_Client()
 chat_server.start()
 text_input = Text_Input()
 text_input.start()
 else:
 chat_server = Chat_Server()
 chat_client = Chat_Client()
 chat_client.host = ip_addr
 text_input = Text_Input()
 chat_client.start()
 text_input.start()
if __name__ == "__main__":
 main()

• Marc-Andre: Welcome to Code Review! There are currently closing votes on your question since you mention problem with it. Note that we do not fix broken code here. As your problem seems like a network problem ( I don't know python to conclude if your code has a problem), I would suggest you edit your question to remove the problem part and just ask for a regular review.
• Mathieu Guindon: @Marc-Andre I'm voting to leave open - firewall issues aren't issues with the code. OP also says it gets the "works on my machine" certificate, I take it as working code ;)

Answer 1

Regular review.

• main is bloated. All the class definitions do net belong there.

• Creating a Chat_Client instance in a server branch (and vice versa) looks strange. My guess it is a result of a Text_Input design, which assumes that both server and client exist. This is of course wrong. A Text_Input instance should have just one "sink" where the text input would go:

  class Text_Input(threading.Thread):
   def __init__(self, sink):
   threading.Thread.__init__(self)
   self.sink = sink
   self.running = 1
   def run(self):
   while self.running == True:
   text = raw_input('')
   self.sink.send(text)
  

  and server initialization becomes

  chat_server = Chat_Server()
  text_input = TextInput(chat_server)
  

  Same goes for a client. Both server and client should implement send method.

• Network initialization should be moved into constructors. After that, run and send method (same for client and server) should be factored out into a separate Chatter class which client and server inherit from.

• What is the purpose of time.sleep(0)? If the system misbehaves without it, then there is a bug to be fixed, not masked. Hint: you should select for outputready only when there is a message to be sent.

Answer 2

Try using a really weird port number as port 8080 is commonly used for a proxy port.

Also, the client doesn't make any effort to verify it indeed connects to the server you want it to. It could very easily have connected to something else that is already listening on 8080, and it will sit there doing nothing basically forever.

You really should .shutdown() and .close() the sockets before you close the clients in order to prevent handle leaks.

I would recommend you come up with a novel SYN SYN-ACK ACK using a unique signature so that if the client connects to something other than the valid server it will close the socket, notify the user, and close. And so that if an invalid Client connects to the server, The server can Disconnect the invalid session, notify the user, and continue waiting for a valid client to connect.