search

vaadin / framework

Vaadin 6, 7, 8 is a Java framework for modern Java web applications.
http://vaadin.com/
Other
1.78k stars 727 forks source link

ConcurrentModificationException from AbstractCommunicationManager #2459

Closed vaadin-bot closed 11 years ago

vaadin-bot commented 12 years ago

Originally by @jdahlstrom

When using AbstractCommunicationManager concurrently (like when using the dontpush addon) the following concurrency bug might pop up.

java.util.ConcurrentModificationException: null
    at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372) ~[na:1.6.0_27]
    at java.util.AbstractList$Itr.next(AbstractList.java:343) ~[na:1.6.0_27]
    at com.vaadin.terminal.gwt.server.AbstractCommunicationManager.getDirtyVisibleComponents(AbstractCommunicationManager.java:2065) ~[vaadin-6.7.8.jar:6.7.8]
    at com.vaadin.terminal.gwt.server.AbstractCommunicationManager.writeUidlResponce(AbstractCommunicationManager.java:988) ~[vaadin-6.7.8.jar:6.7.8]
    at org.vaadin.dontpush.server.BroadcasterVaadinSocket.paintChanges(BroadcasterVaadinSocket.java:131) ~[dontpush-addon-ozonelayer-0.4.8-SNAPSHOT.jar:0.4.8-SNAPSHOT]
    at org.vaadin.dontpu

The issue is quite simple: dirtyPaintables in AbstractCommunicationManager can be modified by multiple threads. If that happens, it will blow up, as ArrayList doesn't support concurrent modification. For example Collections.synchronizedList, or CopyOnWriteArraylist would solve the issue at various overhead costs.

vaadin-bot commented 12 years ago

Originally by @Artur-

Looks like either you or the Dontpush add-on is missing a synchronize(application) somewhere where the UI is being modified

vaadin-bot commented 11 years ago

Originally by ketoth.xupack

Same here. Is AbstractCommunicationManager is designed to be not thread safe?

vaadin-bot commented 11 years ago

Originally by @mstahv

AbstractCommunicationManager is not thread safe by design. People of the world developing Vaadin applications, synchronise UI modifications if they originate from non UI thread or modify UI of another session. So instead of this doStuffWithThings() do something like this:

synchronized(getApplication()) { doStuffWithThings();}

Shame on us as our book examples are broken and apparently this is not well enough emphasised. #7348, #2792 There is no synchronized keyword in the book. Magi, please fix this!

vaadin-bot commented 11 years ago

Originally by iwein

I'm not sure if the implications are entirely clear, so let me clarify. If AbstractCommunicationManager is designed to be not thread safe, it follows that Vaadin is designed not to run on clustered environments. It means Vaadin doesn't scale by design and it means that Vaadin is unreliable by design. I'm OK with that, because nobody is forcing me to use Vaadin. But I think it would be good to put these caveats on the home page, because it can be very costly to have to rewrite your application from scratch once you figure this out.

Session replication works by modifying a session from a different thread than the request thread, and by the design decision not to support that without synchronization, Vaadin is making this hard to do right. The fact that the book, but also Stable add-ons like dontpush-ozonlayer suffer from this flaw, illustrates that Vaadin was really never meant to scale.

If you want to use Vaadin in a cluster, it means you should not use any add-ons unless you've reviewed their code carefully and ensured that they synchronize properly. That takes a lot of discipline...

A better solution imo is to not use HttpSession, which takes Vaadin out of the picture entirely. Being clear and upfront about these considerations would do Vaadin a lot of good.

vaadin-bot commented 11 years ago

Originally by @mstahv

Hi,

The current synchronization solution isn't perfect, but it is dead simple, fast and works in most cases very well. Often even with replicated sessions because it is very uncommon that one user would cause two UI threads simultaneously. With two keyboard or mouses perhaps?

Issues exist if you both have cluster and lots of changes to UI from non UI threads. In this case you might need to build some custom lock solution. I have never done this, but you could most probably build this kind of system rather easily with HttpServletRequestListener ( or with TransactionListeener) and use the same system also from non UI threads. This of course requires you to have a proper locking method that works across the cluster available.

I don't probably understand what you mean by problems related to add-ons? Normal add-ons usually have nothing to do with this issue and AFAIK the OzoneLayer add-on don't cause any issues that wouldn't exist with just core framework. These issues just more often pop up in apps that also need a "server push solution" like OzoneLayer.

Locking sessions is built in bit different way in V7. There VaadinSession class has methods lock() and unlock(). I haven't worked with these V7 renewals, but I hope and believe all this is easier with the next major release.

I completely agree these things should be better discussed/documented somewhere. Concurrency is always a hard topic. Not even the simple synchronization isn't discussed well enough. I didn't check our Pro Account Knowledge Base if there is an entry. If not, at least there should be one.

BTW. For simple clustering solution where synchronized block isn't a problem, check out Terracotta. I haven't played with it myself, but I have heard it has be successfully used with Vaadin.

cheers, matti