NullPointerException during quickfix initialization

[cheremin]

I've met with NPE inside quickfix on initialization. I've seen it in v2.0.0, but apt code in master is still same: https://github.com/quickfix-j/quickfixj/blob/f09e6d63416549a928893d06974aa643935e8919/quickfixj-core/src/main/java/quickfix/DefaultMessageFactory.java#L120

Root cause: Thread.getContextClassLoader() is not guaranteed to be not-null according to spec

@return the context ClassLoader for this Thread, or {@code null} indicating the system class loader (or, failing that, the bootstrap class loader)

In my case it so happens first quickfix call was from network library handler, which is done from network library thread, and such a thread missed context class loader.

(I've also noted factoryClass is not assigned in the catch branch -- i.e. even if context classLoader is not null, factory class will be loaded, but not used!)

I don't understand code deep enough to decide what to do with such a case, but generic NPE is definitely not user-friendly. IMHO:

1. If there is no way to continue without classloader -> it is at least worth to throw ClassNotFoundException with reasonable error description
2. If it is OK to not load specific factory -> than maybe just ignore that factory loading. But it seems to me in such a case none of factories would be loaded (because class loader would be null during whole initialization), without any hint about the reason -- and this could be even more confusing for user to debug.

[the-thing]

I think not many will people see the issue unless class loading is customised. However, you are right. There is a couple of issues here.

1) When quickfix.DefaultMessageFactory class loader is unable to load the class it will fall back to context class loader which either does nothing (see 2), or throws java.lang.NullPointerException. The latter is probably OK as long as the better exception is used.

2) Failing to load a factory might be silent.

I don't know if you are planning to submit a pull request, but I would try to include the following considerations.

1) quickfix.DefaultMessageFactory could support a class loader hint via additional constructor. Default would be current class loader, so compatibility is preserved. On the other hand if the hint is specified we might want to assume that somebody knows where the classes are and we should only try this one.

2) Add utility class/method that would use the first non null class loader in order e.g. hint, current, context, system (or a subset of these). At the moment the default implementation only tries current since the context class loader is buggy and does nothing even if available.

Workaround

Override quickfix.DefaultMessageFactory#addFactory(java.lang.String, java.lang.String) or create a brand new quickfix.MessageFactory implementation.