Stop sending resend-messages when the responder has gone away

[philipwhiuk]

We should probably stop resending messages when the responder has disconnected.

The current behavior is to keep sending messages regardless, ignoring the return value of send().

Here's an anonymised log example.

event INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: Resending message: 3
outgoing INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: <MESSAGE>
event INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: Resending message: 4
outgoing INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: <MESSAGE>
event INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: Resending message: 5
outgoing INFO 20230502-18:15:40.420 - FIXT.1.1:A->B: <MESSAGE>
errorEvent ERROR 20230502-18:15:40.522 - FIXT.1.1:A->B: Disconnecting: Socket exception (/<IP>:<PORT>): java.io.IOException: Connection reset by peer
event INFO 20230502-18:15:40.522 - FIXT.1.1:A->B: Resending message: 6
event INFO 20230502-18:15:40.522 - FIXT.1.1:A->B: No responder, not sending message: <MESSAGE>
event INFO 20230502-18:15:40.741 - FIXT.1.1:A->B: Resending message: 7
event INFO 20230502-18:15:40.741 - FIXT.1.1:A->B: No responder, not sending message: <MESSAGE>
...
event INFO 20230502-18:15:44.741 - FIXT.1.1:A->B: Resending message: 1000
event INFO 20230502-18:15:44.741 - FIXT.1.1:A->B: No responder, not sending message: <MESSAGE>

[philipwhiuk]

I think best fix for this is to return if send returns false, and not send the rest of the resend here:

quickfixj/quickfixj-core/src/main/java/quickfix/Session.java

Line 2378 in dce91e6

send(msg.toString());

But I'd welcome a second opinion.

[chrjohn]

Hi @philipwhiuk , thanks for opening the issue. I think it would be a very sensible enhancement.

AFAIK we do not act upon the return value of the send() method anywhere since it is a leftover from the JNI compatibility and messages are sent async anyway (except if SocketSynchronousWrites is false).

From the line of code that you quoted, we ultimately end up here when the responder is not null:

quickfixj/quickfixj-core/src/main/java/quickfix/Session.java

Line 2774 in dce91e6

return responder.send(messageString);

and from there to here:

quickfixj/quickfixj-core/src/main/java/quickfix/mina/IoSessionResponder.java

Lines 51 to 76 in dce91e6

	public boolean send(String data) {
	// Check for and disconnect slow consumers.
	if (maxScheduledWriteRequests > 0 && ioSession.getScheduledWriteMessages() >= maxScheduledWriteRequests) {
	Session qfjSession = (Session) ioSession.getAttribute(SessionConnector.QF_SESSION);
	try {
	qfjSession.disconnect("Slow consumer", true);
	} catch (IOException e) {
	}
	return false;
	}
	// The data is written asynchronously in a MINA thread
	WriteFuture future = ioSession.write(data);
	if (synchronousWrites) {
	try {
	if (!future.awaitUninterruptibly(synchronousWriteTimeout)) {
	log.error("Synchronous write timed out after {}ms", synchronousWriteTimeout);
	return false;
	}
	} catch (RuntimeException e) {
	log.error("Synchronous write failed: {}", e.getMessage());
	return false;
	}
	}
	return true;
	}

This method might also return false in two cases but I think it would be OK to abort the resend process in both cases since we can assume that the connection is either broken (and disconnected anyway) or the sync write timed out for one message and it would not make sense to resend the next message since at least one message was skipped over which subsequently would lead to another gap on the other side.

So in general it looks good to me. 👍

P.S.: Maybe we should open a follow-up issue to check if it is sensible to act upon the return value of send() in other cases.