From: Artur Signell Date: Mon, 16 Nov 2009 10:51:48 +0000 (+0000) Subject: Merged test case and fix for #3184 - "TransactionListener addition/iteration can... X-Git-Tag: 6.7.0.beta1~2294 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=6a4c80e831867488ee9fc58a5e2af04a6922f80f;p=vaadin-framework.git Merged test case and fix for #3184 - "TransactionListener addition/iteration can cause ConcurrentModificationException" from 6.1 svn changeset:9813/svn branch:6.2 --- diff --git a/src/com/vaadin/service/ApplicationContext.java b/src/com/vaadin/service/ApplicationContext.java index c5bafa8a6d..308a936335 100644 --- a/src/com/vaadin/service/ApplicationContext.java +++ b/src/com/vaadin/service/ApplicationContext.java @@ -13,7 +13,8 @@ import com.vaadin.Application; /** * ApplicationContext provides information about the running * context of the application. Each context is shared by all applications that - * are open for one user. In web-environment this corresponds to HttpSession. + * are open for one user. In a web-environment this corresponds to a + * HttpSession. * * @author IT Mill Ltd. * @version @@ -25,28 +26,28 @@ public interface ApplicationContext extends Serializable { /** * Returns application context base directory. * - * Typically an application is deployed in a such way that is has + * Typically an application is deployed in a such way that is has an * application directory. For web applications this directory is the root - * directory of the web applications. In some cases application might not - * have application directory (for example web applications running inside - * of war). + * directory of the web applications. In some cases applications might not + * have an application directory (for example web applications running + * inside a war). * - * @return The application base directory + * @return The application base directory or null if the application has no + * base directory. */ public File getBaseDirectory(); /** - * Gets the applications in this context. + * Returns a collection of all the applications in this context. * - * Gets all applications in this context. Each application context contains - * all applications that are open for one user. + * Each application context contains all active applications for one user. * - * @return Collection containing all applications in this context + * @return A collection containing all the applications in this context. */ public Collection getApplications(); /** - * Adds transaction listener to this context. + * Adds a transaction listener to this context. * * @param listener * the listener to be added. @@ -55,7 +56,7 @@ public interface ApplicationContext extends Serializable { public void addTransactionListener(TransactionListener listener); /** - * Removes transaction listener from this context. + * Removes a transaction listener from this context. * * @param listener * the listener to be removed. @@ -64,9 +65,8 @@ public interface ApplicationContext extends Serializable { public void removeTransactionListener(TransactionListener listener); /** - * Interface for listening the application transaction events. - * Implementations of this interface can be used to listen all transactions - * between the client and the application. + * Interface for listening to transaction events. Implement this interface + * to listen to all transactions between the client and the application. * */ public interface TransactionListener extends Serializable { @@ -74,6 +74,11 @@ public interface ApplicationContext extends Serializable { /** * Invoked at the beginning of every transaction. * + * The transaction is linked to the context, not the application so if + * you have multiple applications running in the same context you need + * to check that the request is associated with the application you are + * interested in. This can be done looking at the application parameter. + * * @param application * the Application object. * @param transactionData @@ -85,6 +90,11 @@ public interface ApplicationContext extends Serializable { /** * Invoked at the end of every transaction. * + * The transaction is linked to the context, not the application so if + * you have multiple applications running in the same context you need + * to check that the request is associated with the application you are + * interested in. This can be done looking at the application parameter. + * * @param applcation * the Application object. * @param transactionData diff --git a/src/com/vaadin/terminal/gwt/server/WebApplicationContext.java b/src/com/vaadin/terminal/gwt/server/WebApplicationContext.java index 304282fd9f..e49c95d905 100644 --- a/src/com/vaadin/terminal/gwt/server/WebApplicationContext.java +++ b/src/com/vaadin/terminal/gwt/server/WebApplicationContext.java @@ -36,7 +36,8 @@ import com.vaadin.service.ApplicationContext; public class WebApplicationContext implements ApplicationContext, HttpSessionBindingListener, Serializable { - protected List listeners; + protected List listeners = Collections + .synchronizedList(new LinkedList()); protected transient HttpSession session; @@ -87,7 +88,7 @@ public class WebApplicationContext implements ApplicationContext, } /** - * Gets the application context for HttpSession. + * Gets the application context for an HttpSession. * * @param session * the HTTP session. @@ -108,70 +109,68 @@ public class WebApplicationContext implements ApplicationContext, } /** - * Adds the transaction listener to this context. + * Adds a transaction listener to this context. The transaction listener is + * called before and after each each HTTP request related to this session + * except when serving static resources. + * * * @see com.vaadin.service.ApplicationContext#addTransactionListener(com.vaadin.service.ApplicationContext.TransactionListener) */ public void addTransactionListener(TransactionListener listener) { - if (listeners == null) { - listeners = new LinkedList(); - } listeners.add(listener); } /** - * Removes the transaction listener from this context. + * Removes a transaction listener from this context. The transaction + * listener is called before and after each each HTTP request related to + * this session except when serving static resources. * * @see com.vaadin.service.ApplicationContext#removeTransactionListener(com.vaadin.service.ApplicationContext.TransactionListener) */ public void removeTransactionListener(TransactionListener listener) { - if (listeners != null) { - listeners.remove(listener); - } + listeners.remove(listener); } /** - * Notifies the transaction start. + * Sends a notification that a transaction is starting. * * @param application + * The application associated with the transaction. * @param request - * the HTTP request. + * the HTTP request that triggered the transaction. */ protected void startTransaction(Application application, HttpServletRequest request) { - if (listeners == null) { - return; - } - for (final Iterator i = listeners.iterator(); i.hasNext();) { - ((ApplicationContext.TransactionListener) i.next()) - .transactionStart(application, request); + synchronized (listeners) { + for (TransactionListener listener : listeners) { + listener.transactionStart(application, request); + } } } /** - * Notifies the transaction end. + * Sends a notification that a transaction has ended. * * @param application + * The application associated with the transaction. * @param request - * the HTTP request. + * the HTTP request that triggered the transaction. */ protected void endTransaction(Application application, HttpServletRequest request) { - if (listeners == null) { - return; - } - LinkedList exceptions = null; - for (final Iterator i = listeners.iterator(); i.hasNext();) { - try { - ((ApplicationContext.TransactionListener) i.next()) - .transactionEnd(application, request); - } catch (final RuntimeException t) { - if (exceptions == null) { - exceptions = new LinkedList(); + + synchronized (listeners) { + for (TransactionListener listener : listeners) { + try { + listener.transactionEnd(application, request); + } catch (final RuntimeException t) { + if (exceptions == null) { + exceptions = new LinkedList(); + } + exceptions.add(t); } - exceptions.add(t); } } diff --git a/tests/src/com/vaadin/tests/server/TransactionListenersConcurrency.java b/tests/src/com/vaadin/tests/server/TransactionListenersConcurrency.java new file mode 100644 index 0000000000..d33163ddae --- /dev/null +++ b/tests/src/com/vaadin/tests/server/TransactionListenersConcurrency.java @@ -0,0 +1,185 @@ +package com.vaadin.tests.server; + +import static org.easymock.EasyMock.createMock; + +import java.lang.Thread.UncaughtExceptionHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Properties; +import java.util.Random; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; + +import junit.framework.TestCase; + +import org.easymock.EasyMock; + +import com.vaadin.Application; +import com.vaadin.service.ApplicationContext.TransactionListener; +import com.vaadin.terminal.gwt.server.WebApplicationContext; + +public class TransactionListenersConcurrency extends TestCase { + + /** + * This test starts N threads concurrently. Each thread creates an + * application which adds a transaction listener to the context. A + * transaction is then started for each application. Some semi-random delays + * are included so that calls to addTransactionListener and + * WebApplicationContext.startTransaction are mixed. + * + */ + public void testTransactionListeners() throws Exception { + final List exceptions = new ArrayList(); + + HttpSession session = createSession(); + final WebApplicationContext context = WebApplicationContext + .getApplicationContext(session); + List threads = new ArrayList(); + + for (int i = 0; i < 5; i++) { + Thread t = new Thread(new Runnable() { + + public void run() { + Application app = new Application() { + + @Override + public void init() { + // Sleep 0-1000ms so another transaction has time to + // start before we add the transaction listener. + try { + Thread.sleep((long) (1000 * new Random() + .nextDouble())); + } catch (InterruptedException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } + + getContext().addTransactionListener( + new DelayTransactionListener(2000)); + } + + }; + + // Start the application so the transaction listener is + // called later on. + try { + + app.start(new URL("http://localhost/"), + new Properties(), context); + } catch (MalformedURLException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } + + try { + // Call the transaction listener using reflection as + // startTransaction is protected. + + Method m = context.getClass().getDeclaredMethod( + "startTransaction", Application.class, + HttpServletRequest.class); + m.setAccessible(true); + m.invoke(context, app, null); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + }); + + threads.add(t); + t.setUncaughtExceptionHandler(new UncaughtExceptionHandler() { + + public void uncaughtException(Thread t, Throwable e) { + e = e.getCause(); + exceptions.add(e); + } + }); + } + + // Start the threads and wait for all of them to finish + for (Thread t : threads) { + t.start(); + } + int running = threads.size(); + + while (running > 0) { + for (Iterator i = threads.iterator(); i.hasNext();) { + Thread t = i.next(); + if (!t.isAlive()) { + running--; + i.remove(); + } + } + } + + for (Throwable t : exceptions) { + if (t instanceof InvocationTargetException) { + t = t.getCause(); + } + t.printStackTrace(System.err); + fail(t.getClass().getName()); + } + + System.out.println("Done, all ok"); + + } + + /** + * Creates a HttpSession mock + * + */ + private static HttpSession createSession() { + HttpSession session = createMock(HttpSession.class); + EasyMock.expect( + session.getAttribute(WebApplicationContext.class.getName())) + .andReturn(null).anyTimes(); + session.setAttribute( + EasyMock.eq(WebApplicationContext.class.getName()), EasyMock + .anyObject()); + + EasyMock.replay(session); + return session; + } + + /** + * A transaction listener that just sleeps for the given amount of time in + * transactionStart and transactionEnd. + * + */ + public static class DelayTransactionListener implements TransactionListener { + + private int delay; + + public DelayTransactionListener(int delay) { + this.delay = delay; + } + + public void transactionStart(Application application, + Object transactionData) { + try { + Thread.sleep(delay); + } catch (InterruptedException e) { + e.printStackTrace(); + } + + } + + public void transactionEnd(Application application, + Object transactionData) { + try { + Thread.sleep(delay); + } catch (InterruptedException e) { + e.printStackTrace(); + } + + } + } + +}