Session logic doesn't account for a failure when calling messageStore methods (e.g. persisting a message)

[mgatny]

https://github.com/quickfixgo/quickfix/blob/master/session.go#L235

In the qf/c++ and qf/j impls, an error is logged to the event log and sendToTarget returns false

[cbusbey]

What about IncrNextTargetMsgSeqNum?

[bhaan]

I would like to add a correction here. Looking at the implementation of qf/c++ and qf/j, while sendToTarget does return a bool value indicating the success or failure of the sent message, the persistence of that message does not effect that return value. Instead, both implementations throw IO Exceptions in the case of a persistence failure: qf/c++: https://github.com/quickfix/quickfix/blob/master/src/C%2B%2B/Session.cpp#L592-L593 qf/j: https://github.com/quickfix-j/quickfixj/blob/master/quickfixj-core/src/main/java/quickfix/MessageStore.java#L43 and https://github.com/quickfix-j/quickfixj/blob/master/quickfixj-core/src/main/java/quickfix/Session.java#L2485-L2489

Additionally, the qfix/go implementation complicates this issue because the message store persistence (and the actual sending of the message) happens on a separate goroutine, where sendToTarget simply adds the message to a queue. While qfix/c++ and qfix/j appear to perform these actions within the sendToTarget call stack.

The implication here is that callers of sendToTarget in the c++/j versions are able to catch the IO Exception and handle accordingly. If we want to offer similar behavior in qfix/go, we would need to communicate that error asynchronously across goroutines (ideally) via channels.

[bhaan]

I would like to correct my correction 😅 I appear to have missed the big try / catch block around sending messages: https://github.com/quickfix/quickfix/blob/master/src/C%2B%2B/Session.cpp#L528-L532

Nevertheless, the fact that these actions are happening on a separate goroutine still adds difficulty to this problem.

[mgatny]

while sendToTarget does return a bool value indicating the success or failure of the sent message, the persistence of that message does not effect that return value.

sendRaw catches IOException, logs it to event log, and returns false

[mgatny]

Additionally, the qfix/go implementation complicates this issue because the message store persistence (and the actual sending of the message) happens on a separate goroutine, where sendToTarget simply adds the message to a queue.

Yes this is a known problem (although not yet as a github issue afaik), and causes unsent messages if the app is shut down/crashes

[cbusbey]

@bhaan is this closed with #164 ?

[bhaan]

yes it does. I forgot to associate the PR with this issue. Thanks for catching this