Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 3.1
-
Fix Version/s: 3.2
-
Component/s: None
-
Labels:None
-
Environment:ICEpush environment
-
Assignee Priority:P1
Description
Test with the Auction sample with the java test client. Typical test involves 30 clients joining the auction push group. During the test startup phase I generally get one or two of the following Exceptions:
java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
at java.util.AbstractList$Itr.next(AbstractList.java:343)
at org.icepush.LocalNotificationBroadcaster.broadcast(LocalNotificationBroadcaster.java:100)
at org.icepush.LocalPushGroupManager$Notification.run(LocalPushGroupManager.java:364)
at org.icepush.LocalPushGroupManager$QueueConsumerTask.run(LocalPushGroupManager.java:448)
at java.util.TimerThread.mainLoop(Timer.java:512)
at java.util.TimerThread.run(Timer.java:462)
12-Sep-2012 2:39:34 PM org.icepush.servlet.AsyncAdaptingServlet <init>
The application runs fine after the startup phase. It's unclear whether the client actually was able to join the group or not.
Looking at the code, the broadcast method iterates over the set of receivers
for (Receiver receiver : receivers) {
receiverConfirmedMapLock.lock();
try {
if (receiverConfirmedMap.get(receiver) == ConfirmationStatus.FALSE) {
receiver.receive(notifiedPushIds, receiverConfirmationMap.remove(receiver));
}
} finally {
receiverConfirmedMapLock.unlock();
}
}
while the addReceiver method can freely add new receivers to the underlying collection:
public void addReceiver(Receiver receiver) {
receivers.add(receiver);
}
java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
at java.util.AbstractList$Itr.next(AbstractList.java:343)
at org.icepush.LocalNotificationBroadcaster.broadcast(LocalNotificationBroadcaster.java:100)
at org.icepush.LocalPushGroupManager$Notification.run(LocalPushGroupManager.java:364)
at org.icepush.LocalPushGroupManager$QueueConsumerTask.run(LocalPushGroupManager.java:448)
at java.util.TimerThread.mainLoop(Timer.java:512)
at java.util.TimerThread.run(Timer.java:462)
12-Sep-2012 2:39:34 PM org.icepush.servlet.AsyncAdaptingServlet <init>
The application runs fine after the startup phase. It's unclear whether the client actually was able to join the group or not.
Looking at the code, the broadcast method iterates over the set of receivers
for (Receiver receiver : receivers) {
receiverConfirmedMapLock.lock();
try {
if (receiverConfirmedMap.get(receiver) == ConfirmationStatus.FALSE) {
receiver.receive(notifiedPushIds, receiverConfirmationMap.remove(receiver));
}
} finally {
receiverConfirmedMapLock.unlock();
}
}
while the addReceiver method can freely add new receivers to the underlying collection:
public void addReceiver(Receiver receiver) {
receivers.add(receiver);
}
Activity
- All
- Comments
- History
- Activity
- Remote Attachments
- Subversion
Sending src/main/java/org/icepush/LocalNotificationBroadcaster.java
Transmitting file data .
Committed revision 31159.
Show
Jack Van Ooststroom
added a comment - - edited Sending src/main/java/org/icepush/LocalNotificationBroadcaster.java
Transmitting file data .
Committed revision 31159.
The LocalNotificationBroadcaster now uses a CopyOnWriteArrayList for managing its Receivers. This ensures that during iteration, add and removes from outside don't affect the Iterator. Marking this one as FIXED.
Show
Jack Van Ooststroom
added a comment - - edited The LocalNotificationBroadcaster now uses a CopyOnWriteArrayList for managing its Receivers. This ensures that during iteration, add and removes from outside don't affect the Iterator. Marking this one as FIXED.
Jack made some changes and sent me a jar to test. It's not quite completely perfectly possible to test because the javatestclient doesn't work against the current trunk code because of some protocol changes, the result of which I don't think it's possible to be completely sure that the code is completely coherent as a product.
That said, the changed jar file that Jack provided me seems to solve this issue. While it's not possible to prove a negative, I didn't see a single exception once in 5 launches of 60 clients.
One thing I did see was an increase in the amount of time that it took to launch the test. Testing against the original application, it takes the test client 2 minutes to launch 60 clients, on one of the remote Amazon cloud servers, with the javatestclient trying to connect as fast as possible.
Against the new code, the javatestclient takes about 3 minutes to start the test. I would expect using locking synchronization, it will become hard to add a new client if the push application is doing a lot of pushing and spending a lot of time receiving confirmations, both conditions that will be true in Auction, on one of the slow-to-access amazon cloud servers.
Jack mentioned there are changes afoot in this area. I would say that whoever makes the changes should be aware of these types of issues in this area. We can't constantly lock the list of active users whenever there's push activity (via the broadcast method) and still expect clients to be able to join the broadcast list.
(Restricted to icesoft-internal-developers group)