From 9f6bceab2861e4d5a8bccca6c2283804def6477c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Leif=20=C3=85strand?= Date: Fri, 7 Sep 2012 14:57:53 +0300 Subject: [PATCH] Use dedicated Lock for session synchronization (#9156) --- .../server/AbstractCommunicationManager.java | 40 +++++++++++++++---- .../src/com/vaadin/server/VaadinPortlet.java | 5 ++- .../src/com/vaadin/server/VaadinSession.java | 22 ++++++++-- .../application/ThreadLocalInstances.java | 5 ++- .../RowUpdateShouldRetainContextMenu.java | 5 ++- .../table/TableFirstRowFlicker.java | 5 ++- .../MassInsertMemoryLeakTestApp.java | 29 +++++++++----- 7 files changed, 87 insertions(+), 24 deletions(-) diff --git a/server/src/com/vaadin/server/AbstractCommunicationManager.java b/server/src/com/vaadin/server/AbstractCommunicationManager.java index 00d508de4c..c2a5377e12 100644 --- a/server/src/com/vaadin/server/AbstractCommunicationManager.java +++ b/server/src/com/vaadin/server/AbstractCommunicationManager.java @@ -300,9 +300,12 @@ public abstract class AbstractCommunicationManager implements Serializable { cleanStreamVariable(owner, variableName); } } catch (Exception e) { - synchronized (session) { + session.getLock().lock(); + try { handleChangeVariablesError(session, (Component) owner, e, new HashMap()); + } finally { + session.getLock().unlock(); } } sendUploadResponse(request, response); @@ -345,9 +348,12 @@ public abstract class AbstractCommunicationManager implements Serializable { cleanStreamVariable(owner, variableName); } } catch (Exception e) { - synchronized (session) { + session.getLock().lock(); + try { handleChangeVariablesError(session, (Component) owner, e, new HashMap()); + } finally { + session.getLock().unlock(); } } sendUploadResponse(request, response); @@ -379,10 +385,13 @@ public abstract class AbstractCommunicationManager implements Serializable { filename, type, contentLength); try { boolean listenProgress; - synchronized (session) { + session.getLock().lock(); + try { streamVariable.streamingStarted(startedEvent); out = streamVariable.getOutputStream(); listenProgress = streamVariable.listenProgress(); + } finally { + session.getLock().unlock(); } // Gets the output target stream @@ -403,10 +412,13 @@ public abstract class AbstractCommunicationManager implements Serializable { if (listenProgress) { // update progress if listener set and contentLength // received - synchronized (session) { + session.getLock().lock(); + try { StreamingProgressEventImpl progressEvent = new StreamingProgressEventImpl( filename, type, contentLength, totalBytes); streamVariable.onProgress(progressEvent); + } finally { + session.getLock().unlock(); } } if (streamVariable.isInterrupted()) { @@ -418,8 +430,11 @@ public abstract class AbstractCommunicationManager implements Serializable { out.close(); StreamingEndEvent event = new StreamingEndEventImpl(filename, type, totalBytes); - synchronized (session) { + session.getLock().lock(); + try { streamVariable.streamingFinished(event); + } finally { + session.getLock().unlock(); } } catch (UploadInterruptedException e) { @@ -427,20 +442,26 @@ public abstract class AbstractCommunicationManager implements Serializable { tryToCloseStream(out); StreamingErrorEvent event = new StreamingErrorEventImpl(filename, type, contentLength, totalBytes, e); - synchronized (session) { + session.getLock().lock(); + try { streamVariable.streamingFailed(event); + } finally { + session.getLock().unlock(); } // Note, we are not throwing interrupted exception forward as it is // not a terminal level error like all other exception. } catch (final Exception e) { tryToCloseStream(out); - synchronized (session) { + session.getLock().lock(); + try { StreamingErrorEvent event = new StreamingErrorEventImpl( filename, type, contentLength, totalBytes, e); streamVariable.streamingFailed(event); // throw exception for terminal to be handled (to be passed to // terminalErrorHandler) throw new UploadException(e); + } finally { + session.getLock().unlock(); } } return startedEvent.isDisposed(); @@ -551,7 +572,8 @@ public abstract class AbstractCommunicationManager implements Serializable { // The rest of the process is synchronized with the session // in order to guarantee that no parallel variable handling is // made - synchronized (session) { + session.getLock().lock(); + try { // Finds the UI within the session if (session.isRunning()) { @@ -592,6 +614,8 @@ public abstract class AbstractCommunicationManager implements Serializable { paintAfterVariableChanges(request, response, callback, repaintAll, outWriter, uI, analyzeLayouts); postPaint(uI); + } finally { + session.getLock().unlock(); } outWriter.close(); diff --git a/server/src/com/vaadin/server/VaadinPortlet.java b/server/src/com/vaadin/server/VaadinPortlet.java index 45a952d48d..026873a04d 100644 --- a/server/src/com/vaadin/server/VaadinPortlet.java +++ b/server/src/com/vaadin/server/VaadinPortlet.java @@ -529,7 +529,8 @@ public class VaadinPortlet extends GenericPortlet implements Constants { // Finds the window within the application UI uI = null; - synchronized (application) { + application.getLock().lock(); + try { if (application.isRunning()) { switch (requestType) { case RENDER: @@ -555,6 +556,8 @@ public class VaadinPortlet extends GenericPortlet implements Constants { } // if window not found, not a problem - use null } + } finally { + application.getLock().unlock(); } // TODO Should this happen before or after the transaction diff --git a/server/src/com/vaadin/server/VaadinSession.java b/server/src/com/vaadin/server/VaadinSession.java index f0e3ec4895..c6b2d91f29 100644 --- a/server/src/com/vaadin/server/VaadinSession.java +++ b/server/src/com/vaadin/server/VaadinSession.java @@ -28,6 +28,8 @@ import java.util.LinkedList; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import javax.portlet.PortletSession; @@ -76,6 +78,8 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { .findMethod(BootstrapListener.class, "modifyBootstrapPage", BootstrapPageResponse.class); + private final Lock lock = new ReentrantLock(); + /** * An event sent to {@link #start(SessionStartEvent)} when a new Application * is being started. @@ -929,15 +933,17 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { return uI; } Integer uiId = getUIId(request); - - synchronized (this) { + getLock().lock(); + try { uI = uIs.get(uiId); if (uI == null) { uI = findExistingUi(request); } - } // end synchronized block + } finally { + getLock().unlock(); + } UI.setCurrent(uI); @@ -1290,4 +1296,14 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { return Collections.unmodifiableCollection(uiProviders); } + /** + * Gets the lock that should be used to synchronize usage of data inside + * this session. + * + * @return the lock that should be used for synchronization + */ + public Lock getLock() { + return lock; + } + } diff --git a/uitest/src/com/vaadin/tests/application/ThreadLocalInstances.java b/uitest/src/com/vaadin/tests/application/ThreadLocalInstances.java index 3adb284f1b..688b2ea4f5 100644 --- a/uitest/src/com/vaadin/tests/application/ThreadLocalInstances.java +++ b/uitest/src/com/vaadin/tests/application/ThreadLocalInstances.java @@ -34,8 +34,11 @@ public class ThreadLocalInstances extends AbstractTestCase { Thread thread = new Thread() { @Override public void run() { - synchronized (ThreadLocalInstances.this) { + getSession().getLock().lock(); + try { reportCurrentStatus("background thread"); + } finally { + getSession().getLock().unlock(); } } }; diff --git a/uitest/src/com/vaadin/tests/components/table/RowUpdateShouldRetainContextMenu.java b/uitest/src/com/vaadin/tests/components/table/RowUpdateShouldRetainContextMenu.java index a9d5869e5f..44fc77d142 100644 --- a/uitest/src/com/vaadin/tests/components/table/RowUpdateShouldRetainContextMenu.java +++ b/uitest/src/com/vaadin/tests/components/table/RowUpdateShouldRetainContextMenu.java @@ -36,8 +36,11 @@ public class RowUpdateShouldRetainContextMenu extends TestBase { sleep(1000); } catch (InterruptedException ie) { } - synchronized (RowUpdateShouldRetainContextMenu.this) { + getContext().getLock().lock(); + try { indicator.setValue(progress += 0.01); + } finally { + getContext().getLock().unlock(); } } } diff --git a/uitest/src/com/vaadin/tests/components/table/TableFirstRowFlicker.java b/uitest/src/com/vaadin/tests/components/table/TableFirstRowFlicker.java index 5d36b8381b..f937ba6ed5 100644 --- a/uitest/src/com/vaadin/tests/components/table/TableFirstRowFlicker.java +++ b/uitest/src/com/vaadin/tests/components/table/TableFirstRowFlicker.java @@ -43,7 +43,8 @@ public class TableFirstRowFlicker extends LegacyApplication { @Override public void run() { while (t != null) { - synchronized (t.getUI().getSession()) { + t.getUI().getSession().getLock().lock(); + try { int firstId = t.getCurrentPageFirstItemIndex(); Object selected = t.getValue(); t.setContainerDataSource(buildContainer()); @@ -51,6 +52,8 @@ public class TableFirstRowFlicker extends LegacyApplication { t.setCurrentPageFirstItemIndex(firstId); // lighter alternative for all of above // t.refreshRowCache(); + } finally { + t.getUI().getSession().getLock().unlock(); } try { Thread.sleep(500); diff --git a/uitest/src/com/vaadin/tests/containers/sqlcontainer/MassInsertMemoryLeakTestApp.java b/uitest/src/com/vaadin/tests/containers/sqlcontainer/MassInsertMemoryLeakTestApp.java index 3a07a89302..f3bd903686 100644 --- a/uitest/src/com/vaadin/tests/containers/sqlcontainer/MassInsertMemoryLeakTestApp.java +++ b/uitest/src/com/vaadin/tests/containers/sqlcontainer/MassInsertMemoryLeakTestApp.java @@ -46,13 +46,18 @@ public class MassInsertMemoryLeakTestApp extends LegacyApplication { private class MassInsert extends Thread { @Override - public synchronized void start() { - proggress.setVisible(true); - proggress.setValue(new Float(0)); - proggress.setPollingInterval(100); - process.setEnabled(false); - proggress.setCaption(""); - super.start(); + public void start() { + getContext().getLock().lock(); + try { + proggress.setVisible(true); + proggress.setValue(new Float(0)); + proggress.setPollingInterval(100); + process.setEnabled(false); + proggress.setCaption(""); + super.start(); + } finally { + getContext().getLock().unlock(); + } } @Override @@ -73,11 +78,14 @@ public class MassInsertMemoryLeakTestApp extends LegacyApplication { getRandonName()); } c.commit(); - synchronized (MassInsertMemoryLeakTestApp.this) { + getContext().getLock().lock(); + try { proggress .setValue(new Float((1.0f * cent) / cents)); proggress.setCaption("" + 100 * cent + " rows inserted"); + } finally { + getContext().getLock().unlock(); } } } catch (SQLException e) { @@ -87,10 +95,13 @@ public class MassInsertMemoryLeakTestApp extends LegacyApplication { e.printStackTrace(); } } - synchronized (MassInsertMemoryLeakTestApp.this) { + getContext().getLock().lock(); + try { proggress.setVisible(false); proggress.setPollingInterval(0); process.setEnabled(true); + } finally { + getContext().getLock().unlock(); } } } -- 2.39.5