[QFJ-572] RuntimeExceptions cause messages to be silently dropped Created: 04/Feb/11 Updated: 14/Nov/12 Resolved: 14/Nov/12 |
|
Status: | Resolved |
Project: | QuickFIX/J |
Component/s: | Engine |
Affects Version/s: | 1.5.0 |
Fix Version/s: | 1.5.3 |
Type: | Improvement | Priority: | Default |
Reporter: | Stelios Papadopoulos | Assignee: | Christoph John |
Resolution: | Fixed | Votes: | 2 |
Labels: | None |
Description |
An Invalid argument exception was thrown from methods invoked in fromApp() java.lang.IllegalArgumentException: message 8=FIXT.1.1^A9=480^A35=d^A34=7528 ... This caused QuickFixJ to think that it never received the message. So on the next message the sequence was out of order: 2011-02-04 07:41:21,737 [QFJ Message Processor] INFO quickfixj.event - FIXT.1.1:IGMM-MD->NDXMD: MsgSeqNum too high, expecting 7528 but received 7529 QuickFixJ then requested the message again. The Exception was caused by the contents of the message, so on reception of the offending message the same thing happened again. I believe that the default behaviour should be to reject the message indicating some kind of application failure, or something else other than silently dropping the message which can cause a resend loop. |
Comments |
Comment by Thomas Wölfle [ 20/Sep/11 ] |
We had exactly the same problem. In our case in a Groovy Class, which was a subclass of ApplicationAdapter, an exception has been thrown that resulted in the described behaviour. In our case it was not a RuntimeException but a CheckedException. Since Groovy does not distinguish between runtime and checked exceptions the exception was simply thrown up the call stack resulting in the described behaviour above. I.e. perhaps not only RuntimeExceptions but all Throwables have to be catched in 'quickfix.Session.next' |
Comment by Christoph John [ 20/Dec/11 ] |
I think this would be a good enhancement. Basically, the behaviour is in-line with the spec which says: The tricky part here might be to discover an infinite resend loop. But as a first step it might be a good improvement to generate a Reject or BusinessMessageReject message if there is a problem inside Session.next(). This should be configurable (on/off). |
Comment by Grant Birchmeier [ 09/Oct/12 ] |
Given Christoph's quote of the spec, it sounds like QF/J is only lacking the loop detection, correct? Aside from that, it's behaving appropriately. An option to generate a reject seems like merely a workaround, and not an actual solution. I'm more interested in the actual reject cause - why the hell would FromApp() be throwing an IllegalArgumentException? That sounds like a bug somewhere. Same for Thomas' report – why would message-parsing throw a CheckedException? These are exceptions that imply coding problems. |
Comment by Thomas Wölfle [ 09/Oct/12 ] |
As described in my initial comment the ApplicationAdapter in our System can be extended by Groovy classes. We do this in order to be able to dynamically add code to patch incoming or outgoing FIX messages via Groovy scripts. In such a Groovy script it might happen that a Java method is called that throws a checked exception. Catching checked exceptions is verified by the Java compiler but since Groovy is a dynamic language there is no compilation that verifies that no such methods are called. This results in checked exceptions that are thrown but never caught. |
Comment by Christoph John [ 10/Oct/12 ] |
I can understand Grant's point as well as Thomas's. Maybe the configurable rejection can be a workaround for the time being and we will add the infinite-loop-detection in a later version (suggestions on how to dectect this are welcome ). |
Comment by Thomas Wölfle [ 11/Oct/12 ] |
We have solved our problem using the following patch which simply adds another catch block in the method Session.next(Message). It is almost the same code as in the catch block for 'FieldNotFound' exceptions. — core/src/main/java/quickfix/Session.java (revision 1052) + } catch(final QuickfixRuntimeException e) { + if (sessionID.getBeginString().compareTo(FixVersions.BEGINSTRING_FIX42) >= 0 else { else { + generateReject(message, "Application not available"); + }+ }········ + } |
Comment by Christoph John [ 11/Oct/12 ] |
Thanks for your input, but I was more referring to a suggestion for the loop-detection. |
Comment by Christoph John [ 14/Nov/12 ] |
Committed as rev #1101.
|