The Robot World Repo on GitHub
The Forth Repo on GitHub

Let’s use the message length to decide when to respond. I think we can make this happen. Also: Love one another.

I think I’ll try this testing approach: we’ll provide the message length on the way in, and expect it to have been removed on the response. Let’s change the tests:

    def test_one_go_round(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            msg = b"Hello, world"
            prefixed = prefix(msg)
            s.sendall(prefixed)
            data = s.recv(1024)
        assert data == msg

    def test_two_messages(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            s.sendall(prefix(b"Hello, world"))
            data = s.recv(1024)
            assert data == "Hello, world".encode('utf-8')
            s.sendall(prefix(b'How ya doing?'))
            data = s.recv(1024)
            assert data == "How ya doing?".encode('utf-8')

These all fail with similar messages:

Expected :b'Hello, world'
Actual   :b'\x00\x0cHello, world'

Now over in the MessageHandler, we’ll need to strip off the count and use it. (I am tempted to make the test harder, but this will do for now. We’ll talk about it after this much works.)

After realizing that unpack expects to be able to unpack all the bytes it’s given, this works:

class MessageHandler:
    def __init__(self, addr):
        self.addr = addr
        self.inb = b""
        self.outb = b""
        self.size = None

    def process_read(self, sel, sock):
        self.inb += sock.recv(1024)  # Should be ready to read
        if self.size is None:
            if len(self.inb) >= 2:
                self.size = struct.unpack('>H', self.inb[:2])[0]
                self.inb = self.inb[2:]
        if self.size is not None and len(self.inb) >= self.size:
            self.outb = self.inb[:self.size]
            self.inb = self.inb[self.size:]
            self.size = None
        else:
            print(f"Closing connection {self}")
            sel.unregister(sock)
            sock.close()

Let me explain that to myself, and, if you care to listen, to you:

We init our buffers empty and self.size to None. self.size will be the number of bytes in the actual message, which is stored as a half-word integer in the two-byte prefix. So on a read, we receive bytes into inb, and if we do not know size, and we have at least two bytes, we unpack the size. (My defect was that I originally passed in the entire inb, not just the first two bytes as we see now.)

Then, if we have a size and we have enough bytes, we move all the bytes (but not the size, which has been consumed, to outb, where it is sent back by the process_write method (not shown).)

We should now be able to add out “I saw:” and make it work. Change the tests like this:

    def test_two_messages(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            s.sendall(prefix(b"Hello, world"))
            data = s.recv(1024)
            assert data == "I saw: Hello, world".encode('utf-8')
            s.sendall(prefix(b'How ya doing?'))
            data = s.recv(1024)
            assert data == "I saw: How ya doing?".encode('utf-8')

Make them work:

    def process_read(self, sel, sock):
        self.inb += sock.recv(1024)  # Should be ready to read
        if self.size is None:
            if len(self.inb) >= 2:
                self.size = struct.unpack('>H', self.inb[:2])[0]
                self.inb = self.inb[2:]
        if self.size is not None and len(self.inb) >= self.size:
            self.outb = b"I saw: " + self.inb[:self.size]
            self.inb = self.inb[self.size:]
            self.size = None
        else:
            print(f"Closing connection {self}")
            sel.unregister(sock)
            sock.close()

Eek, I got so excited I forgot to commit. We’re green: Server honors message length on input, echoes “I saw: “ plus input message.

I think we should be able to break up the message into pieces now and have everything work fine. Let’s try it:

    def test_two_messages(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            s.sendall(prefix(b"Hello,"))
            s.sendall(b" world")
            data = s.recv(1024)
            assert data == "I saw: Hello, world".encode('utf-8')
            s.sendall(prefix(b'How ya doing?'))
            data = s.recv(1024)
            assert data == "I saw: How ya doing?".encode('utf-8')

I am quite surprised when that fails:

Expected :b'I saw: Hello, world'
Actual   :b'I saw: Hello,'

Ah, the test is wrong, I have to sent the full length and I just calculated the prefix on the short message.

    def test_two_messages(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            msg = prefix(b"Hello, world")
            s.sendall(msg[:6])
            s.sendall(msg[6:])
            data = s.recv(1024)
            assert data == "I saw: Hello, world".encode('utf-8')
            s.sendall(prefix(b'How ya doing?'))
            data = s.recv(1024)
            assert data == "I saw: How ya doing?".encode('utf-8')

Green. Commit: test partial messages.

Let’s make a harder one, just sending half the prefix.

    def test_two_messages(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            msg = prefix(b"Hello, world")
            s.sendall(msg[:6])
            s.sendall(msg[6:])
            data = s.recv(1024)
            assert data == "I saw: Hello, world".encode('utf-8')
            how_ya = prefix(b'How ya doing?')
            s.sendall(how_ya[:1])
            s.sendall(how_ya[1:4])
            s.sendall(how_ya[4:])
            data = s.recv(1024)
            assert data == "I saw: How ya doing?".encode('utf-8')

This fails on a closed connection. My read is wrong:

    def process_read(self, sel, sock):
        self.inb += sock.recv(1024)  # Should be ready to read
        if self.size is None:
            if len(self.inb) >= 2:
                self.size = struct.unpack('>H', self.inb[:2])[0]
                self.inb = self.inb[2:]
        if self.size is not None and len(self.inb) >= self.size:
            print(f'{self.size=} {self.inb=} {len(self.inb)=}')
            self.outb = b"I saw: " + self.inb[:self.size]
            self.inb = self.inb[self.size:]
            self.size = None
        else:
            print(f"Closing connection {self}")
            sel.unregister(sock)
            sock.close()

That else is in the wrong place entirely. Try this:

    def process_read(self, sel, sock):
        self.inb += sock.recv(1024)  # Should be ready to read
        if not self.inb:
            print(f"Closing connection {self}")
            sel.unregister(sock)
            sock.close()
            return
        if self.size is None:
            if len(self.inb) >= 2:
                self.size = struct.unpack('>H', self.inb[:2])[0]
                self.inb = self.inb[2:]
        if self.size is not None and len(self.inb) >= self.size:
            print(f'{self.size=} {self.inb=} {len(self.inb)=}')
            self.outb = b"I saw: " + self.inb[:self.size]
            self.inb = self.inb[self.size:]
            self.size = None

And we are green. Commit: fix to close only on zero bytes read. No! Not yet … what about a short record. We need to check the count read this time.

    def process_read(self, sel, sock):
        bytes = sock.recv(1024)  # Should be ready to read
        if not bytes:
            print(f"Closing connection {self}")
            sel.unregister(sock)
            sock.close()
            return
        self.inb += bytes
        if self.size is None:
            if len(self.inb) >= 2:
                self.size = struct.unpack('>H', self.inb[:2])[0]
                self.inb = self.inb[2:]
        if self.size is not None and len(self.inb) >= self.size:
            print(f'{self.size=} {self.inb=} {len(self.inb)=}')
            self.outb = b"I saw: " + self.inb[:self.size]
            self.inb = self.inb[self.size:]
            self.size = None

Still green. Now the commit: fix to close only on zero bytes read. I was saved by my commit message, which reminded me that I was checking the input buffer, not bytes read.

I’m not sure if I can test that or not, and I’m tired. I’ll try once.

    def test_incomplete_message(self):
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
            s.connect(("127.0.0.1", 34567))
            msg = prefix(b"Hello, world")
            s.sendall(msg[:6])
            s.setblocking(False)
            with pytest.raises(BlockingIOError) as e:
                data = s.recv(1024)

This test does pass, telling us, maybe, that there was no data ready. I think in general one handles that error by ignoring it in a real non-blocking situation. Anyway: commit test partial message and close raises exception.

This is good progress, let’s sum up.

Summary

We’ve arranged our MessageHandler class to expect a message length in the first two bytes of the message, and it only produces output when the full message has been read. We’ve tested partial messages, including only getting half the message length, which I suspect is almost impossible, and including closing the connection before completing the necessary count.

We are pretty close to the ability to respond other than by what is essentially an echo. I think you can see that once we have the message in inb we can look it up, pass it to an app, whatever. We’ll do that next time.

I did see one issue that I don’t understand. At one point in the proceedings above, I started getting messages about socket already in use. I didn’t chase that, I just exited PyCharm and restarted it. I wonder if there is some small number of sockets available and whether crashing or stopping the server used them up. I’ll have to look into that, do a little research.

We have working code, and I think we’ll see that we could use a little refactoring. That will happen Real Soon Now.

Overall, this is good stuff: we are progressing in the direction of a real game server, and so far I think I understand the code.

Love one another and write your congresspersons. Impeach Trump. Again