RosterGroup.addEntry does not work in Smack 2.0

Looks like RosterGroup.addEntry doesn’'t work in Smack 2.0 (latest sources)

Here is the test:

private static void testSetGroup() throws XMPPException {

XMPPConnection c = new XMPPConnection(“localhost”);

try {

try {

c.getAccountManager().createAccount(“bob”, “ddd”);

c.getAccountManager().createAccount(“alice”, “ddd”);

}

catch(Exception e){ // already exists?}

c.login(“bob”, “ddd”);

Roster bobRoster = c.getRoster();

bobRoster.createEntry(“alice@localhost”, “Alice”, new String[0]);

RosterGroup someGroup = bobRoster.createGroup(“someGroup”);

RosterEntry entry = bobRoster.getEntry(“alice@localhost”);

Assert.assertNotNull(entry);

someGroup.addEntry(entry);

Assert.assertTrue(someGroup.contains(entry));

c.close();

c = new XMPPConnection(“localhost”);

c.login(“bob”, “ddd”);

Assert.assertNotNull(c.getRoster().getGroup(“someGroup”));

} finally {

c.getAccountManager().deleteAccount();

}

}

/pre

The last assertion in this test fails.

To fix the problem, addEntry should be rewritten this way:

public void addEntry(RosterEntry entry) throws XMPPException {

PacketCollector collector = null;

// Only add the entry if it isn’'t already in the list.

synchronized (entries) {

if (!entries.contains(entry)) {

RosterPacket packet = new RosterPacket();

packet.setType(IQ.Type.SET);

RosterPacket.Item item = RosterEntry.toRosterItem(entry);

// Add this group name

item.addGroupName(getName());

packet.addRosterItem(item);

// Wait up to a certain number of seconds for a reply from the server.

collector = connection

.createPacketCollector(new PacketIDFilter(packet.getPacketID()));

connection.sendPacket(packet);

}

}

if (collector != null) {

IQ response = (IQ) collector.nextResult(SmackConfiguration.getPacketReplyTimeout());

collector.cancel();

if (response == null) {

throw new XMPPException(“No response from the server.”);

}

// If the server replied with an error, throw an exception.

else if (response.getType() == IQ.Type.ERROR) {

throw new XMPPException(response.getError());

}

// Add the new entry to the group since the server processed the request successfully

addEntryLocal(entry);

}

}

/pre

Another issue is, that to prevent ConcurrentModificationException (I got it sometimes in multithreaded environment), the last statements in addEntry and removeEntry methods should use addEntryLocal and removeEntryLocal instead of plain access to ‘‘entries’’ list.

With kind regards,

KIR

Hey maxkir,

This sounds surprisingly similar to my post from last week: http://www.jivesoftware.org/community/thread.jspa?threadID=16112&tstart=15 .

The fact that nobody mentioned this bug in 2-3 years, and now you do one week after I did amuses me greatly.

Dear Javier,

Actually, I didn’‘t see your post. Honestly. And, actually, I’‘ve discovered the bug after upgrading from smack 1.5 to 2.0 version, when one of my tests fail. I’‘m sorry I didn’'t scan newsgroups before posting the issue.

Regarding addEntryLocal(entry) note - I actually got ConcurrentModificationException because of absense of this call. Again, I didn’'t see your post.

What amuses me - why the issue isn’'t fixed in smack codebase yet.

With kind regards,

KIR

Looks to me like you are experiencing a race condition. The packet is sent to the server for the entry to be added, when the server returns the packet the roster packet parser inside of the roster then adds the entry. Your test code is just beating it to the punch, or getting there at the same time when you get the ConcurrentModificationException. Your solution, adding the entry directly inside of the #addEntry() method, is inconsitent with the way Smack currently handles Rosters, for the most part. A more appropriate solution is to listen for updates on the Roster, the #rosterModified() method specificly, and do your assertion then.

Looks to me like you are experiencing a race

condition. The packet is sent to the server for the

entry to be added, when the server returns the packet

The real[/b] problem is, that the sent packet does not

contain information about the group to be added to the user.

(No call item.addGroupName(getName()); in original sources)

the roster packet parser inside of the roster then

adds the entry. Your test code is just beating it to

the punch, or getting there at the same time when you

get the ConcurrentModificationException. Your

ConcurrentModificationException i got with another test.

That’'s why i replace the unsynchronized call

entries.add(entry) with addLocalEntry(entry),

which contains synchronized block.

solution, adding the entry directly inside of the

#addEntry() method, is inconsitent with the way

It is not my solution, but Smack developers’'. I just made the

call synchronized.

Smack currently handles Rosters, for the most part. A

more appropriate solution is to listen for updates on

the Roster, the #rosterModified() method specificly,

and do your assertion then.

Agreed. That’'s how I write most of my tests. But addEntry call IS

synchronous by design, so I add assertion right after it.

Kind regards,

KIR

Hmm, yeah, i defintly looked at that one wrong. I said that looking at the original source of the Roster#addEntry() which confused me. But with all that said I completely agree with you that there are some problems with the RosterGroup#addEntry() as it is currently layed out. It would be my opinion that the design should be completely changed to be asyncronous and consolidated into the roster code, a protected method perhaps? The end point being that there are other problems that creep in when you attempt to make the call synchronized, as it is. For instance, as we discussed in this thread: http://www.jivesoftware.org/community/thread.jspa?threadID=16177&tstart=0

In this case, the server is returning a different packet number as a result then is sent. If this call is kept as is this will cause an exception to be raised even when the roster entry is in fact added. A solution i have thought about in these cases and to give adecuete feedback to the end user would be to create a listener on the roster a roster error perhaps, which keeps track of roster queries to the server and responds with an error when something goes wrong, perhaps when entries aren’‘t added when they are supposed to be, the server doesn’'t respond, etc. This may make a little more effort for the client developer in the end but it would hopefully mitigate, the confusion that the current layout creates.

I should add that that I completly agree with the fact that RosterGroup#addEntry does not add the current group. Whoops.

IMO, there should be an additional parameter in addEntry method to indicate, whether the call should be synchronous or not. As well as for other similar methods.

Anyway, I’'d like to see the bugs fixed in the Smack SVN repository. Matt, Gaston, any feedback on this?

Hey guys,

I think we recently just ran into a similar issue. We’‘ll file an issue and get it fixed ASAP. We’'ll post an update in this thread when a JIRA issue number is available.

-Matt

Heh, whoops. Gato actually already filed and fixed SMACK-92.

-Matt