-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Functional adapters for Application
and ApplicationExtended
interfaces
#212
Conversation
Hi, it's clean code. But what about the MessageCraker and type safe style ? |
This pull request is mainly concerned about the use of functional interfaces. I could have a look at being type safe as the next step. Although it was highly recommended to choose the most type safe way to handle quickfix message, it should not be mandatory. And if the users wish to, they could mix in MessageCracker in the FromAppListener implementation by something like this: new FromAppMessageCracker(new FromAppListenerImpl()); MessageCracker will scan the first parameter and register the invokers to be called when the type is matched. |
If we were to attempt to use MessageCracker as type-safe delegation for fromApp, toApp, fromAdmin and toAdmin, the first thing we'll find is that we'll need to pass in java.lang.Object to have its methods scanned on "onMessage" and the "@handler" annotation. And lambda expressions no longer work because the compiler expects an interface as lambda's target. Then how about generic type?, something like public class FromAppListener { but due to Erasure of java generics, there is no way to resolve what T actually is at runtime. The only workaround I could think of is to provide the Class when adding the listener. It would look like this: adapter.addFromAppListener(NewOrderSingler.class, (NewOrderSingle m, SessionID s) -> {}); and the adapter will keep a Map<Class, List> for type safe delegation. In this usage, there is no need to use MessageCracker. No need to conform to "onMessage" or annotate the method with "@handler". To conclude this, I think I could at least modify the custom interfaces to have T extends Message in preparation for the type-safe delegation. And the actual type-safe implementation should not be too far ahead, if you agree adding the Class in the argument of listener registration is acceptable. |
I have added support for type-safe listeners for toApp, fromApp, toAdmin and fromAdmin, as mentioned in my previous comment. |
Hi @jasition , thanks for the PR. Since your changes do not break anything they should be good to go. However, I'll wait some time if anyone does a review. Cheers, |
Thank you Chris. If this is going to be accepted, I would update the receiving_messages.html to add the usage example. |
Yes, that would be great. If it is not too much effort you could also add a short example to the top-level README.md. But I could also do that later on. |
…eceiving_messages.html
I have added the example to README.md and receiving_messages.html. However, feel free to modify, organise and integrate the new section with other sections in the document however you like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I think this was very good work and I appreciate the additional functionality. Good job on the test coverage. Most of my comments were grammatical, which is not hugely fair as English isn't your first language, though your fluency is impressive. There are a few comments of more substance. The only thing that I would really like to see changed is the definitions of your collections switched from specific classes to interfaces.
README.md:
- Missing space: then
ApplicationFunctionalAdapter
- Grammar: They also allows -> They also allow
- registration of the same event -> registration to the same event
receiving_messages.html
- If you prefer to using -> If you prefer to use
- register reaction to events -> register reactions to events
- They also allows registering the interests to -> They also allow registering interest in
- support multiple registration of the same event -> support multiple registration to the same event
ApplicationExtendedFunctionalAdapter
- ApplicationExtended interface, and that transforms -> ApplicationExtended interface that transforms
- supplied by lambda expressions -> supplied with lambda expressions
- This comment: "by the means of CopyOnWriteArrayList" breaks encapsulation. The comment in general is good because it warns callers that there is a potential performance issue ("under the assumption...") and what the method accomplishes ("provides a thread-safe..."), however, the comment should not say how this is accomplished. Again, the idea is that if you were to change the implementation in the future (e.g. change to a synchronized list, which I'm not suggesting, this is just an example), the comment should not have to change. Also, the phrase in English is, "by means of", not "by the means of".
- Declare canLogonPredicates and onBeforeSessionResetListeners as List instead of CopyOnWriteArrayList. The declaration should be an interface to allow easy changes in the future to the implementation in case you want to use a different collection type.
ApplicationFunctionalAdapter.java
- Application interface, and that transforms -> Application interface that transforms
- supplied by lambda expressions -> supplied with lambda expressions
- type safe -> type-safe
- Again, breaks encapsulation, and I recommend removing this sentence fragment: by the means of concurrent and immutable collections,
- Declare with interfaces instead of implementations:
-- CopyOnWriteArrayList -> List
-- ConcurrentHasMap -> Map
-- ConcurrentLinkedQueue -> Queue/Deque - getQueue - I don't see the benefit of your implementation here vs just synchronizing multimap. I am concerned that a small future modification in the while loop could cause an infinite loop. Better(?) to just:
synchronized(multimap) {
Queue queue = multimap.get(clazz);
if(queue == null) {
queue = new ConcurrentLinkedQueue<>();
multimap.put(clazz,queue);
}
return queue;
} - It would be nice to see more type-safety in the various collections, e.g.: "private final Map<Class, Queue<BiConsumer<? extends Message,SessionID>>> toAdminTypeSafeListeners = new ConcurrentHashMap<>()". I understand why you didn't, because it becomes more difficult to adapt to "onAdmin(Message)" etc. This isn't anything that needs to be addressed, it's just a pedantic wish on my part.
Thanks for your comment. Having reviewed the cases, I realise that we would only create a new queue for the message type when
With the double-checked locking technique, I could make it lock-free if the queue already exists. A new pull request is coming with all the grammar corrections. The mistakes in README.md and receiving_messages.html were the same since the content was the same. So I will correct both. |
…d map; Use synchronized block when creating type-safe list
quickfixj-core/src/main/java/quickfix/ApplicationFunctionalAdapter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Hi @charlesbr1, I haven't encountered the infinite loop in my experience. But I could see how |
Hi @jasition , @colinduplantis , @charlesbr1 thanks for your work and comments. Much appreciated. Cheers, |
Is there an example of an application that uses this? I've searched pretty hard and can't find one. It looks neat and I think it might be useful in my app, but... the readme is just not quite enough to get me there. I feel like I've got all the pieces in front of me, but I don't know how to put them together. An example would help a lot. |
Application
and ApplicationExtended
interfaces
I guess you've already looked at the test https://github.com/quickfix-j/quickfixj/blob/7ad10b92bfdb606e043bbb0ca54169f61af5293c/quickfixj-core/src/test/java/quickfix/ApplicationFunctionalAdapterTest.java The adapter implements the |
I did not see that test, thanks. Not sure how I missed it. I'd still like to see a concrete instantiation of FromAppListener, and an example of how I'll have to hack up a demo app and experiment. Does AFA still throw UnsupportedMessageType if no listener has a function assigned to handle a message type? I don't see a test that covers that. |
I think I get it now, mostly. I was given some misleading partial source from a client that was leading me down the wrong kind of thought. (In other words, they are not quite using it as intended.) |
Provides adapter implementation of quickfix.Application and quickfix.ApplicationExtended that
allows registering multiple listeners for each operation (e.g. onLogon, onCreate) separately using Java 8's functional interfaces like Consumer, BiConsumer and Predicate. FromAdmin, FromApp and ToApp requires custom interfaces due to checked Exceptions. Also allows removing registered listeners.
invokes each registered listeners in a FIFO manner
for Predicate, it fail-fast when encounters the first Predicate returning false
for checked Exception listeners, it fail-fast when encounters the first Exception thrown
is completely optional to Quickfix users
is backward compatible
is thread-safe with the use of CopyOnWriteArrayList
does not add extra lib dependency