Python socket error - recv() function

Question

I've been trying to code a simple chat server in Python, my code is as follows:

import socket
import select
port = 11222
serverSocket = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
serverSocket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1024)
serverSocket.bind(('',port))
serverSocket.listen(5)
sockets=[serverSocket]
print 'Server is started on port' , port,'\n'
def acceptConn():
 newsock, addr = serverSocket.accept()
 sockets.append(newsock)
 newsock.send('You are now connected to the chat server\n')
 msg = 'Client joined',addr.__str__(),
 broadcast(msg, newsock)
def broadcast(msg, sourceSocket):
 for s in sockets:
 if (s != serverSocket and s != sourceSocket):
 s.send(msg)
 print msg,
while True:
 (sread, swrite, sexec)=select.select(sockets,[],[])
 for s in sread:
 if s == serverSocket:
 acceptConn()
 else:
 msg=s.recv(100) 
 if msg.rstrip() == "quit":
 host,port=socket.getpeername()
 msg = 'Client left' , (host,port)
 broadcast(msg,s)
 s.close()
 sockets.remove(s)
 del s
 else:
 host,port=s.getpeername()
 msg = s.recv(1024)
 broadcast(msg,s)
 continue

After running the server and connecting via telnet, the server reads single character and skips the next one. Example if I type Hello in telnet, server reads H l o. Any help please ?! :)

• abarnert: Not your actual problem here, but you need to change socket.getpeername() to s.getpeername() here, and also handle clients that drop connection without sending "quit" first.
• Ahmed Mohamed: I actually used socket instead of s to get the list of socket members and forgot to rechange it.

Answer 1

You call recv twice.

First:

msg=s.recv(100)

Then, if that's not "quit", you read and broadcast another message:

msg = s.recv(1024)
broadcast(msg,s)

So the original message is lost.

Because you're using telnet as the client, you get one character at a time, so you see every other character. If you used, say, nc instead, you'd get different results—but still the same basic problem of every other read being thrown away.

There are a few other problems here:

• You're expecting clients to send "quit" before quitting—you should be handling EOF or error from recv and/or passing sockets in the x as well as the r.
• You're assuming that "quit" will always appear in a single message, and an entire message all to itself. This is not a reasonable assumption with TCP. You may get four 1-byte reads of "q", "u", "i", and "t", or you may get a big read of "OK, bye everyone\nquit\n", neither of which will match.
• The "Client left" and "Client joined" messages are tuples, not strings, and they're formed differently, so you're going to see ('Client joined', "('127.0.0.1', 56564)") ('Client left', ('127.0.0.1', 56564)).
• You're relying on the clients to send newlines between their messages. First, as mentioned above, even if they did, there's no guarantee that you'll get complete/discrete messages. Second, your "system" messages don't have newlines.

Here's a modified version of your sample that fixes most of the problems, except for requiring "quit" to be in a single message alone and relying on the clients to send newlines:

#!/usr/bin/python
import socket
import select
import sys
port = 11222
serverSocket = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
serverSocket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1024)
serverSocket.bind(('',port))
serverSocket.listen(5)
sockets=[serverSocket]
print 'Server is started on port' , port,'\n'
def acceptConn():
 newsock, addr = serverSocket.accept()
 sockets.append(newsock)
 newsock.send('You are now connected to the chat server\n')
 msg = 'Client joined: %s:%d\n' % addr
 broadcast(msg, newsock)
def broadcast(msg, sourceSocket):
 for s in sockets:
 if (s != serverSocket and s != sourceSocket):
 s.send(msg)
 sys.stdout.write(msg)
 sys.stdout.flush()
while True:
 (sread, swrite, sexec)=select.select(sockets,[],[])
 for s in sread:
 if s == serverSocket:
 acceptConn()
 else:
 msg=s.recv(100)
 if not msg or msg.rstrip() == "quit":
 host,port=s.getpeername()
 msg = 'Client left: %s:%d\n' % (host,port)
 broadcast(msg,s)
 s.close()
 sockets.remove(s)
 del s
 else:
 host,port=s.getpeername()
 broadcast(msg,s)
 continue

To fix the 'quit' problem, you're going to have to keep a buffer for each client, and do something like this:

buffers[s] += msg
if '\nquit\n' in buffers[s]:
 # do quit stuff
lines = buffers[s].split('\n')[-1]
buffers[s] = ('\n' if len(lines) > 1 else '') + lines[-1]

But you've still got the newline problem. Imagine that user1 logs in and types "abc\n" while user2 logs in and types "def\n"; you may get something like "abClient joined: 127.0.0.1:56881\ndec\nf\n".

If you want a line-based protocol, you have to rewrite your code to do the echoing on a line-by-line instead of read-by-read basis.

• Ahmed Mohamed: Thanks, I removed the second recv and it works, but still it reads as I type (without pressing enter) and there are whitespaces after each character. Any help? - Thanks just read your edit. If you could provide some code I would be grateful :)
• Ahmed Mohamed: Seems I will re-design the code again, keeping your Points in my head! Thanks for your elegant and comprehensive answer :)
• abarnert: It reads as you type because your telnet is sending one byte at a time, so each read is one byte. (Across a real network, the packets may or may not get merged, but with localhost, each one will invariably show up as a separate read.) The whitespace after each character is because the comma explicitly prints a space instead of a newline; if you want to print neither space nor newline, use sys.stdout.write instead of print.
• Ahmed Mohamed: Thanks a lot abarnert quite useful :)
• abarnert: One last thing: That "with localhost, each one will invariably show up as a separate read" isn't actually so invariable. It depends on how your platform implements localhost sockets; there's nothing in any standard that says they must do it that way. Hopefully that didn't mislead you. Anyway, the important point is that across a real network, it's completely unpredictable how the writes and reads correspond, so you can't rely on whatever you happen to see with small localhost writes.