OpenSSL and concurrent read/write

[kbriggs]

I've got a client/server gaming app where I use an Indy TCPServer component (ver 10.6.3.4) for my packet exchange system via websocket. I've hand-coded my own websocket protocol since it's not native to Indy. Currently my OnExecute loop calls a function that sends any outbound packets that are ready in a queue and then reads the next inbound packet like this:

Code:

// loop here to send any ready outgoing packets
// now check for incoming packet 
if IO.InputBufferIsEmpty and not(IO.CheckForDataOnSource) then Exit;
Opcode := IO.ReadByte;  // start of packet

At the bottom of my OnExecute is a Sleep(10) command to keep the CPU from spinning to 100%.

All this has worked fine for many years now but there is a limit to how many connections (a few hundred) this system can support. I'm having Claude Code look at this and it's telling me the Sleep command is the main bottleneck as the system will eventually spend most of its time thread switching rather than running the game. It initially suggested using a blocking read command with a timeout (allowing me to delete the Sleep) and then moving the outgoing packet writes to a separate worker thread. That all sounded good until we got into the details and now it's telling me "OpenSSL isn't safe for truly concurrent SSL read / SSL write on the same connection without locking". So is this true? Will I have to implement critical sections or MREW locks to prevent any chance of a simultaneous read/write? Putting a lock around the blocking read is highly problematic since it may stay for awhile and that kind of defeats the efficiency improvement I'm trying to make. By the way, I know my Indy install is nearly 2 years out of date. Updating is on my to-do list.

[rlebeau]

Code:

// now check for incoming packet 
if IO.InputBufferIsEmpty and not(IO.CheckForDataOnSource) then Exit;

On a side note, you should call CheckForDisconnect() after CheckForDataOnSource(), eg:

Code:

// now check for incoming packet 
if IO.InputBufferIsEmpty then
begin
  IO.CheckForDataOnSource;
  IO.CheckForDisconnect;
  if IO.InputBufferIsEmpty then Exit;
end;

All this has worked fine for many years now but there is a limit to how many connections (a few hundred) this system can support.

You are really only limited by available memory. TIdTCPServer uses 1 thread per connection, and the default thread stack size is 1-4MB, depending on your project settings. If you are running out of memory for threads, you can try lowering the stack size.

I'm having Claude Code look at this and it's telling me the Sleep command is the main bottleneck as the system will eventually spend most of its time thread switching rather than running the game.

That is true. Try using Sleep(0) instead. Alternatively, don't sleep at all. Indy uses blocking socket I/O. It will sleep the calling thread for you while reading/writing data. Or, at the very least, sleep only when there is no data present, ie when InputBufferIsEmpty is true. If there is data, you usually want the next iteration to read the next data immediately and not delay.

It initially suggested using a blocking read command with a timeout (allowing me to delete the Sleep)

Indy already does that. If you want a timeout, use the IOHandler's ReadTimeout property, or the ATimeout parameter of CheckForDataOnSource(), etc.

and then moving the outgoing packet writes to a separate worker thread.

That is certainly an option, IF your OnExecute handler only reads and never writes. If that is the case, then it can just block on ReadByte() and you won't need CheckForDataOnSource().

That all sounded good until we got into the details and now it's telling me "OpenSSL isn't safe for truly concurrent SSL read / SSL write on the same connection without locking". So is this true?

Yes. In OpenSSL, a read operation may also perform socket writes, and a write operation may also perform socket reads. This typically happens only during handshakes, session renegotiations, alerts, etc. But it does mean you need to be careful about overlapping your I/O operations. A write on one thread may read packets that another thread is waiting for.

[kbriggs]

While I'm waiting for a reply I had another thought. This whole thing could be simplified if there was an TEvent type of feature built into the Indy ReadTimeout system. Then I could set ReadTimeout to a large number like 1000 (or even Indefinite) and remove the Sleep. Then leave the outbound write check where it is now and the first ReadByte below it, waiting up to 1 second (or forever) for the next incoming packet. But when another thread was adding something to the outbound queue it would call the equivalent of TEvent.SetEvent to break the ReadByte out of its wait period. Is something like possible? What if another thread set ReadTimeout to 0 (or another small number), would that work? I could then reset ReadTimeout back to 1000 and run the next loop.

---

Sorry, your reply didn't show until after I posted again but my new question still stands. The memory is not an issue, it's the constant thread switching that slows the whole game when the login capacity is high. I'm trying to maximize that while staying with an Indy thread based system rather than rewriting the whole thing in Node.js. Going with a blocking read only works if I can safely do writes in another thread OR break out of the read timeout when I need to. It appears that the SSL issue eliminates that first option.

---

If I delete the original Sleep(10) and do this instead:

Code:

if IO.InputBufferIsEmpty then
begin
  IO.CheckForDataOnSource;
  IO.CheckForDisconnect;
  if IO.InputBufferIsEmpty then
  begin
    Sleep(0);
    Exit;
  end;
end;

Wouldn't I still get a 100% CPU spike when nothing much else was going on?

[kbriggs]

I ended up just going with a blocking read with the timeout set to 100 msec and trapping the timeout exception. Then deleting the unnecessary buffer check and sleep commands. Seems to work better now.

[rlebeau]

This whole thing could be simplified if there was an TEvent type of feature built into the Indy ReadTimeout system... Is something like possible?

Not consistently in a cross-platform way. You didn't say which platform(s) you are targeting.

On Windows, while there are ways to use event objects with sockets, they only work with non-blocking sockets or overlapped I/O, neither of which Indy uses.

On Nix systems, where sockets are file descriptors, you could create a separate pipe and then select() on both the pipe and socket at the same time. Then write to the pipe when needed.

What if another thread set ReadTimeout to 0 (or another small number), would that work? I could then reset ReadTimeout back to 1000 and run the next loop.

That won't work if a read is already in progress. Also, ReadTimeout=0 is treated the same as ReadTimeout=IdTimeInfinite anyway.

Going with a blocking read only works if I can safely do writes in another thread

That only works if you use plain sockets without TLS. The issue is not limited to OpenSSL specifically, but the protocol itself. Even if you disable renegotiations and such, alerts are still an issue at least, although they are only used at the end of a connection, so maybe not much of an issue.

OR break out of the read timeout when I need to. It appears that the SSL issue eliminates that first option.

And the use of blocking sockets eliminates the second.

If I delete the original Sleep(10) and do this instead:

Code:

if IO.InputBufferIsEmpty then
begin
  IO.CheckForDataOnSource;
  IO.CheckForDisconnect;
  if IO.InputBufferIsEmpty then
  begin
    Sleep(0);
    Exit;
  end;
end;

Wouldn't I still get a 100% CPU spike when nothing much else was going on?

No. But if you are worried about it, you can stick with Sleep(10) (or even Sleep(1)). The point is to sleep only when actually needed (ie, during idle activity), not on every event/packet.

I ended up just going with a blocking read with the timeout set to 100 msec and trapping the timeout exception. Then deleting the unnecessary buffer check and sleep commands. Seems to work better now.

If you stick with the buffer check, you can specify a timeout on it:

Code:

if IO.InputBufferIsEmpty then
begin
  IO.CheckForDataOnSource(10);
  IO.CheckForDisconnect;
  if IO.InputBufferIsEmpty then Exit;
end;

[kbriggs]

If you stick with the buffer check, you can specify a timeout on it:

Code:

if IO.InputBufferIsEmpty then
begin
  IO.CheckForDataOnSource(10);
  IO.CheckForDisconnect;
  if IO.InputBufferIsEmpty then Exit;
end;

That is cleaner than trapping the timeout exception although I'd still probably stick with the 100 msec wait (IO.CheckForDataOnSource(100)) because the problem I'm having is the OnExecute thread spin gobbling the CPU when there are many hundreds of connections. But I also don't want to go too high (like 1000 msec) because then the user starts to notice the response latency.