Race condition when fetching initial roster

Smack Smack Dev
1

Hi,

I am currently testing smack 4.1.2 and encountered an exception when testing consecutive connection and disconnections

for (int i=1; i<50; i++) {

conn.connect();

if (!conn.isAuthenticated())

conn.login(“myUser”, “password”, “smack4test”);

conn.disconnect();

}

It seems that there is a race condition so that the roster implementation waits from a response of an already closed connection (connection closed before the roster was completely fetched?).

Jul 03, 2015 10:18:53 AM org.jivesoftware.smack.roster.Roster$3 processException

SEVERE: Exception reloading roster

org.jivesoftware.smack.SmackException$NoResponseException: No response received within reply timeout. Timeout was 5000ms (~5s). Used filter: IQReplyFilter: iqAndIdFilter (AndFilter: (OrFilter: (IQTypeFilter: type=error, IQTypeFilter: type=result), StanzaIdFilter: id=5y342-7)), : fromFilter (OrFilter: (FromMatchesFilter (full): null, FromMatchesFilter (bare): myUser@myserver, FromMatchesFilter (full): myserver)).

at org.jivesoftware.smack.SmackException$NoResponseException.newWith(SmackExceptio n.java:106)

at org.jivesoftware.smack.AbstractXMPPConnection$6.run(AbstractXMPPConnection.java :1443)

at java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source)

at java.util.concurrent.FutureTask.run(Unknown Source)

at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201 (Unknown Source)

at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknow n Source)

at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)

at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)

at java.lang.Thread.run(Unknown Source)

Best Regards,

Ingo

2

I don’t think I would call that a race condition. The roster request is just not cancelled by disconnect().

What could be improved is that instead of a NoResponseException, a NotConnectedException is thrown.

3

No matter how we name this timing problem, you mean that this is the expected behavior? From an API user’s point of view I would say that I don’t care about this failing of fetching the roster due to the quick disconnect. This exception in the logs pointing out a severe problem are a little bit annoying. Just as background information - I am using the snippet to do a quick check if the server and account settings are correct.

Wouldn’t it better to cancel all of these internal response handlers in case of a disconnect?

4

I’ve changed the code in Throw NotConnectedException instead of NoResponseException · Flowdalic/Smack@bfdcfba · GitHub 4f to change the log level to debug in that case.

Wouldn’t it better to cancel all of these internal response handlers in case of a disconnect?
No, it’s better to throw a NotConnectedException in this case, instead of a NoResponseException. That’s what the commit above does. (Note that the “better” here is relative, the commit above just avoids the bookkeeping required to be able to invoke the exception callbacks and cancel the collectors. I think it’s a good tradeoff, and therefore “better”).

5

Thanks. Now I can filter out the unwanted exception traces. But there was one thing that came into my mine while examining your patch. Couldn’t you check the connection status in the ExceptionCallback of the Roster implementation?

6

Couldn’t you check the connection status in the ExceptionCallback of the Roster implementation?
Well that would be a race condition then, because the connection status could have changed after the exception is thrown but before it is catched (The current impl is still racy, but to a much smaller degree).