Handle KeepAlive messages in a separate Thread (Indy TIdTCPClient)

[VTTB 0]

Good day,

 

I'm supposed to communicate with a Server that receives XML messages and sends XML data back, occasionally (every 30 seconds when there were no other messages passed) the server sends a KeepAlive XML message without the client asking for it, but the client has to answer to the message. So I have a TReceiveThread class, derived from TThread, that gets passed the TIdTCPClient which was created in the main thread aswell as a TCriticalSection for synchronization, implemented .Execute to check once a second if a KeepAlive message arrived and sends an answer back to the server.

.Execute (pseudo code):

.Execute
	while not Terminated
		TThread.Sleep(1000)
		lock.enter
		try
			try
				recevied data (exception on timeout)
				if message type is keepalive
					send ack message
			except
			end
		finally
			lock.leave
		end

So far so good.

 

Now I want to send other messages, for example on a button click from the main thread (pseudo code): 

 

create xml message
lock.enter
try
	try
		send xml message
		read data (exception on timeout)
		parse data
	except
		report timeout error
	end
finally
	lock.leave
end

This also works with every message I've implemented.

However, when I'm not sending a message for 30 seconds so the server sends a KeepAlive message that gets handled and answered in the thread, the next message I'm trying to send from the main thread will throw a EIdReadTimeout (I can see the data sent in Wireshark, but not the data received). When I then send the same message again I'm getting an answer. I've extensively checked for synchronization errors and dead data in the send/receive buffers, that all seems to be fine. 

 

when I change the code above to:

create xml message
lock.enter
try
	try
		try
			read data (exception on timeout) <------ 
		except
		end

		send xml message
		read data (exception on timeout)
		parse data
	except
		report timeout error
	end
finally
	lock.leave
end

as if the first read in the main thread somehow changes the context so the second read works as expected

 

any advice?

 

 

[Remy Lebeau 1636]

You have two threads that are wanting to read at the same time, and write at the same time.  This is not a good design choice.  It is a race between the threads, you don't know which thread will actually receive which message, so BOTH threads have to be ready to reply to KeepAlives, and BOTH threads have to be ready to handle non-KeepAlive replies.  And worse, it is possible that one thread may read bytes that the other thread is expecting.  It is just not a good situation to be in.

 

I would suggest a completely different approach.  Have 2 worker threads - 1 thread whose sole task is to read ALL incoming packets, and 1 thread whose sole task is to write ALL outgoing packets.  When the reading thread receives a message, if the message is a KeepAlive then the reading thread can ask the writing thread to send a reply, otherwise the reading thread can dispatch the message for processing as needed (which probably shouldn't be done in the main thread, if you can avoid it).  When the main thread (or any other thread) wants to send a message, it can ask the writing thread to send it.

 

This way, you don't need to synchronize the reads and writes anymore.  Only synchronize the requests for sending messages.

 

Reading thread:

Reader.Execute
	while not Terminated
		receive data (no timeout)
		if message type is keepalive
			create ack message
			Writer.Send(message)
		else
			dispatch message for processing

Writing thread:

Writer.Send(message)
	queue.lock
	try
		queue.add(message)
		signal.set
	finally
		queue.unlock
	end

Writer.Execute
	while not Terminated
		if signal.wait(timeout)
			try
				queue.lock
				try
					make copy of queue
					queue.clear
					signal.reset
				finally
					queue.unlock
				end;
				send messages in copy
			finally
				copy.clear
			end;

Main thread:

Main.DoSomething
	create xml message
	Writer.Send(message)

Main.DataReceived(data)
	parse data
	if should send next message
		create xml message
		Writer.Send(message)

 

Edited January 28, 2021 by Remy Lebeau

[VTTB 0]

I realize that the proposed solution is probably better but I'm still trying to  figure out what is happening here. I don't see a race between the threads here because they are mutually exclusive through a TCriticalSection, so the Receive Thread can never receive any other message than a KeepAlive because when a regular message is sent to the server the main thread holds the lock until the main thread has received the message, or a timeout occured (here lies the problem, the main thread is not receiving anything unless I read before I send and then read the answer)

 

It also wouldnt be a problem the other way around. If the server would send a KeepAlive just when the Receive Thread has finished it's cycle and I'd then send a regular message from the main thread I should at least receive "garbage" in the main thread, but I receive nothing. Even if there'd be garbage that wouldnt be a problem, I'd just discard the KeepAlive and send the regular message because for the server that's also OK

 

Between KeepAlive messages I have ~30 seconds to send and receive regular messages via the main thread without problems, even though the receive thread is checking for KeepAlive messages once a second and as soon as the receive thread has handled a KeepAlive message (received and answered, buffers are empty afterwards), the first regular message will fail, unless I read *before* I send. If there'd be something wrong with the bytes in the buffers I should at least receive garbage in the main thread, but I dont

 

I will eventually redesign the thing but for now I'm curious why it's not working unless I read (and dont receive anything) in the main thread before sending and then reading again from the main thread.

[Remy Lebeau 1636]

11 hours ago, VTTB said:

I don't see a race between the threads here because they are mutually exclusive through a TCriticalSection, so the Receive Thread can never receive any other message than a KeepAlive

Actually, it can.

 

When TIdIOHandler reads data from a socket, it reads everything that is available on the socket at that moment, and puts the data in its InputBuffer for read methods to then pull from.  So, if there are multiple messages in flight on the connection, the IOHandler could read multiple messages at a time (or even portions of messages).  Whether or not this causes problems for your code, I couldn't say for sure, since you did not show your actual code, only pseudo-code.  IN THEORY, it should be OK, but I don't want to speculate without seeing your real code.

 

But even with your pseudo-code, there is still potential for the Receive Thread to receive messages other than KeepAlives.  If the main thread performs a read and it times out (network lag, etc), the message it was waiting on will not be read in the main thread, so it still will be waiting, and the Receive Thread could then pick it up.

 

Depending on how the messages are formatted, and how you are reading them, there is even potential for the main thread to read only a portion of a message (it could receive some bytes, and then time out on others), and then the Receive Thread could read the remaining portion of the same message.

 

It is really hard to diagnose your issue without actual details about your protocol, and seeing your actual code.

11 hours ago, VTTB said:

because when a regular message is sent to the server the main thread holds the lock until the main thread has received the message

If you happen to send that message near/exactly at the KeepAlive time, the server could send a KeepAlive message before it has a chance to see your new message.  And then your main thread would end up reading the KeepAlive first, and if you unlock at that time then the Receive Thread would read the response that the main thread was expecting.  To avoid that, you would need to have logic in the main thread that loops reading messages inside the lock until a non-KeepAlive message is received.

11 hours ago, VTTB said:

or a timeout occured (here lies the problem, the main thread is not receiving anything unless I read before I send and then read the answer)

That likely indicates the data the main thread is waiting for has actually being read and discarded by the Receive Thread, such as if you are getting your threads out of sync with each other, causing them to read each other's messages (or worse, partial messages).

 

Unsolicited messaging and command/response messaging don't play well together in the kind of design you have chosen.  That is why I suggest you re-write the logic to the kind of design I suggested earlier.

11 hours ago, VTTB said:

It also wouldnt be a problem the other way around. If the server would send a KeepAlive just when the Receive Thread has finished it's cycle and I'd then send a regular message from the main thread I should at least receive "garbage" in the main thread

No, you would receive the KeepAlive message.  Why would you expect to receive garbage?  Unless you mean the processing of the KeepAlive message as a response would be interpreted as garbage in the context of your requested operation.  Then yes, that could happen.  But you definitely don't want actual garbage appearing on your protocol, that would corrupt everything, and you would have to disconnect and start over.

11 hours ago, VTTB said:

but I receive nothing.

The only way that can happen is if either 1) nothing is being sent at all from the server (unlikely, but use a packet sniffer to verify), or 2) the data is being read and discarded somewhere else before you have a chance to read it in the main thread.  Data doesn't just disappear unexpectedly.

11 hours ago, VTTB said:

Even if there'd be garbage that wouldnt be a problem, I'd just discard the KeepAlive and send the regular message because for the server that's also OK.

Yes, but then you would have to detect that KeepAlive message and perform another read to receive the actual response message you were expecting the first time.

11 hours ago, VTTB said:

Between KeepAlive messages I have ~30 seconds to send and receive regular messages via the main thread without problems, even though the receive thread is checking for KeepAlive messages once a second and as soon as the receive thread has handled a KeepAlive message (received and answered, buffers are empty afterwards), the first regular message will fail, unless I read *before* I send.

Again, please show your ACTUAL CODE in order to troubleshoot that.

11 hours ago, VTTB said:

I will eventually redesign the thing but for now I'm curious why it's not working unless I read (and dont receive anything) in the main thread before sending and then reading again from the main thread.

I can't answer that without seeing your ACTUAL PROTOCOL and your ACTUAL CODE.

[VTTB 0]

I've implemented your suggestion, seems to work pretty good, thanks

 

If I'm bored I might try to recreate the problem in a minimal example and post it here