NullPointerException in EntityCapsManager in 4.1.0-rc3

Hello

In the following scenario, I get a npe in EntityCapsManager.

My Application connects to a server, then disconnect, and the reconnect to a different or same server, Then try to start up a private chat, which starts with querying features:

Caused by: java.lang.NullPointerException

at org.jivesoftware.smackx.caps.EntityCapsManager.getDiscoveryInfoByNodeVer(Entity CapsManager.java:203)

at org.jivesoftware.smackx.caps.EntityCapsManager.getDiscoverInfoByUser(EntityCaps Manager.java:187)

at org.jivesoftware.smackx.disco.ServiceDiscoveryManager.discoverInfo(ServiceDisco veryManager.java:484)

at xxxx.queryOnFeatures(JChatBareConnectionImpl.java:421)

Looking in the smack code I get the feeling a check on null is forgotten, but it is strange, that this behavior only after the second connect is present. While debuging, I get the feeling as if the EntityCapsManager is not functioning at all, after the first connect in my scenario, and only after the second connect start to work.

public static DiscoverInfo getDiscoveryInfoByNodeVer(String nodeVer) {

    DiscoverInfo info = CAPS_CACHE.get(nodeVer);

    // If it was not in CAPS_CACHE, try to retrieve the information from persistentCache

    if (info == null) { <==== ? && persistentCache != null )

        info = persistentCache.lookup(nodeVer);

        // Promote the information to CAPS_CACHE if one was found

Should I explicitly assign a persistentCache?

Should the EntityCapsManager perform caching by default?

Thanks for reporting Anno. Was already fixed 4 days ago in Fix NPE in EntityCapsManager · d389b4d · igniterealtime/Smack · GitHub as I discovered the same bug when working on the integration tests for Smack.

I recommend that you use, maybe a pinned version of, 4.1.0-rc4-SNAPSHOT which contains only bugfixes since -rc3 and is likely to become 4.1.0 in the next weeks.

After using the rc-4 the npe was resolved, so thank you.

But still I looked further, trying to find out why this NPE only happened after the second time I connect.

My assumption is now that the EntityCapsManager, when not explicitly called before the connect, is not properly initialized. When the class is called, a connection listener is attached to the connection, but most time this class will be called for the first time after a login, so the code in the connected or authenticated of the ConnectionListener, will only be executed in case a connection is connected a second time.

It is easily solved by calling the EntityCapsManager between configuring and connecting, but i don’t think this is the intention.

A patch will be calling the function processCapsStreamFeatureIfAvailable(connection);

somewhere in the constructor, but not sure about the consequences.

but most time this class will be called for the first time after a login, so the code in the connected orauthenticated of the ConnectionListener, will only be executed in case a connection is connected a second time.

Right, that’s because how the connection creation listeners are called (or more when they are called). There is SMACK-638, which basically says instead of when the connection was the firs time connected, the listeners should get called when the connection is created, i.e. constructed. I’m happy prepare a commit for SMACK-638 and see how it works out, but I’m not keen on making this change in the 4.1 branch. So for 4.1, if you want to have the servers entity caps evaluated right before the first connection, you will have to call EntityCapsManager.getInstanceFor(connection) before connect().

I’ve sketched the change at Call connection created listeners in constructor · 339802f · Flowdalic/Smack · GitHub

but it’s completly untested. It’s possible that calling the connection created listeners with an not completely constructed connection instance could cause some troubles, or at least some unforeseen side effects. But from looking at the code, it appears that it should work.

It’s possible that calling the connection created listeners with an not completely constructed connection instance could cause some troubles, or at least some unforeseen side effects. But from looking at the code, it appears that it should work.

It won’t be thread-safe then. You may want to read IBM Developer about why it’s a bad idea to publish the “this” reference during construction.

I know that it won’t be thread-safe and I’m aware of the implications of publishing an instance while it’s not fully constructed. But I don’t see a problem in this case (although I can’t rule it out, because i’ve not tested the smack638 branch).

This is primary because there are no threads involved. And because the connection created listeners will see all fields, minus some parts of the configuration, initialized when they are called from AbstractXMPPConnection’s constructor. So what’s done typically in a connection creation listener, i.e. adding stanza listeners, iq request handlers, and so on, should work just fine.

If you can sketch a scenario where it breaks, then please share it.

I only see a problem with this from a theoretical point of view, while having these two quotes from JCIP in mind:

You should avoid the temptation to think that there are “special” situations in which this rule does not apply. A program that omits needed synchronization might appear to work, passing its tests and performing well for years, but it is still broken and may fail at any moment.

But an object is in a predictable, consistent state only after its constructor returns, so publishing an object from within its constructor can publish an incompletely constructed object. This is true even if the publication is the last statement in the constructor.

Only wanted to point it out. I am no concurrency expert, but appearently there can be “hidden” (/non-obvious) vulnerabilities.

A scenario might be if a ConnectionCreationListener creates a new Thread (actually doing this from within the Connection’s constructor) and accesses the yet incomplete Connection instance and might face visibility or similar concurrency issues (which I think can’t be circumvented with usual synchronization).

ConnectionCreationListener might not start threads within Smack, but nonetheless it’s public API and we are using it outside of Smack, too. We don’t start threads, but other users might do so.

We don’t start threads, but other users might do so.
I hope those users verify that the API they use is thread safe. The XMPPConnectionRegistry’s connection created listener is not designed to be thread safe, and there appears to be no appealing reason to have it thread safe. All things break if you use a non thread-safe API concurrently.

It’s getting slightly above my concurrency/memory model knowledge now ;-), but I think the problem is, that a Thread which is created within a ConnectionCreationListener may see an incomplete Connection object. Even if the JavaDoc states “not thread-safe”, there’s no way to externally make it thread-safe by the API user, because afaik synchronization can only be applied to e.g. method calls, but not to make an incomplete object complete afterwards.

From java - About reference to object before object’s constructor is finished - Stack Overflow

Even if all of your fields are final in a class, sharing the reference to the object to another thread before the constructor finishes cannot guarantee that the fields have been set by the time the other threads start using the object.

which theoretically may result in NPE, even if accessing a final field, which is assigned before publishing the this reference.

Again, I am not sure about all those memory model stuff, it’s a difficult topic. Maybe I am wrong, but things like that are probably worth to keep in mind.

Although this is interesting discussion about threading, I have the feeling I did not explained correctly, as what I see as the cause of the problem I mentioned.

The CapsEntitiyManager is setup as a lazy initializing manager. This has the effect that even the static code which attaches the manager as a connectionCreationListerner is only then executed when the Class is loaded, on the first time the class is used in the code. In my case this happens a significant time after the login. So I don’t think it is solution to call the listeners somewhere in the constructor of the XMPPConnection, as on this moment, the CapsEntitiyManager class is not yet loaded, and therefore has not yet registered itself as a listener.

I think the only correct solution is, is to make sure that each manager should properly initialize itself, meaning in this case calling the processCapsStreamFeatureIfAvailable(connection); somewhere in the constructor.

I think the only correct solution is, is to make sure that each manager should properly initialize itself,
Such a mechanism exists in Smack: The startup classes.

EntityCapsManager is is simply missing its startup class entry. I didn’t notice that, since I set a persistent cache on startup, which takes care of the initialization (since the class is implicitly loaded and initialized when the store is set). While you should also set a persisent cache, I also added EntityCapsManager as startup class in Add EntityCapsManager to startup classes · 0ca4e8b · igniterealtime/Smack · GitHub