diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
index 96ae9e9dd1c314087865cd2d8c096192978956c8..4274738874c18cdd4042cef321dc361cb56dc1f9 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
@@ -40,6 +40,7 @@ import org.springframework.transaction.annotation.Transactional;
import ch.systemsx.cisd.authentication.IPrincipalProvider;
import ch.systemsx.cisd.authentication.ISessionManager;
import ch.systemsx.cisd.authentication.Principal;
+import ch.systemsx.cisd.common.action.IDelegatedActionWithResult;
import ch.systemsx.cisd.common.exceptions.InvalidSessionException;
import ch.systemsx.cisd.common.exceptions.UserFailureException;
import ch.systemsx.cisd.common.mail.IMailClient;
@@ -753,31 +754,36 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
@SuppressWarnings("deprecation")
@Override
- public void saveDisplaySettings(String sessionToken, DisplaySettings displaySettings,
- int maxEntityVisits)
+ public void saveDisplaySettings(String sessionToken, final DisplaySettings displaySettings,
+ final int maxEntityVisits)
{
try
{
final Session session = getSession(sessionToken);
synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions()
{
- PersonPE person = session.tryGetPerson();
+ final PersonPE person = session.tryGetPerson();
if (person != null)
{
- synchronized (displaySettingsProvider)
- {
- if (maxEntityVisits >= 0)
+ displaySettingsProvider.executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
{
- List<EntityVisit> visits = displaySettings.getVisits();
- sortAndRemoveMultipleVisits(visits);
- for (int i = visits.size() - 1; i >= maxEntityVisits; i--)
+ @Override
+ public Void execute(boolean didOperationSucceed)
{
- visits.remove(i);
+ if (maxEntityVisits >= 0)
+ {
+ List<EntityVisit> visits = displaySettings.getVisits();
+ sortAndRemoveMultipleVisits(visits);
+ for (int i = visits.size() - 1; i >= maxEntityVisits; i--)
+ {
+ visits.remove(i);
+ }
+ }
+ displaySettingsProvider.replaceRegularDisplaySettings(person, displaySettings);
+ getDAOFactory().getPersonDAO().updatePerson(person);
+ return null;
}
- }
- displaySettingsProvider.replaceRegularDisplaySettings(person, displaySettings);
- getDAOFactory().getPersonDAO().updatePerson(person);
- }
+ });
}
}
} catch (InvalidSessionException e)
@@ -788,7 +794,7 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
@Override
public void updateDisplaySettings(String sessionToken,
- IDisplaySettingsUpdate displaySettingsUpdate)
+ final IDisplaySettingsUpdate displaySettingsUpdate)
{
if (displaySettingsUpdate == null)
{
@@ -800,19 +806,24 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
final Session session = getSession(sessionToken);
synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions()
{
- PersonPE person = session.tryGetPerson();
+ final PersonPE person = session.tryGetPerson();
if (person != null)
{
- synchronized (displaySettingsProvider)
- {
- DisplaySettings currentDisplaySettings =
- displaySettingsProvider.getCurrentDisplaySettings(person);
- DisplaySettings newDisplaySettings =
- displaySettingsUpdate.update(currentDisplaySettings);
- displaySettingsProvider.replaceCurrentDisplaySettings(person,
- newDisplaySettings);
- getDAOFactory().getPersonDAO().updatePerson(person);
- }
+ displaySettingsProvider.executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
+ {
+ @Override
+ public Void execute(boolean didOperationSucceed)
+ {
+ DisplaySettings currentDisplaySettings =
+ displaySettingsProvider.getCurrentDisplaySettings(person);
+ DisplaySettings newDisplaySettings =
+ displaySettingsUpdate.update(currentDisplaySettings);
+ displaySettingsProvider.replaceCurrentDisplaySettings(person,
+ newDisplaySettings);
+ getDAOFactory().getPersonDAO().updatePerson(person);
+ return null;
+ }
+ });
}
}
} catch (InvalidSessionException e)
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
index 875eb41101b9ce982ce04cb4212013ad32ac0f25..c0752fb63a715ad2df089b6fa7626f78978a4fc7 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
@@ -16,9 +16,13 @@
package ch.systemsx.cisd.openbis.generic.server;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import ch.systemsx.cisd.common.action.IDelegatedActionWithResult;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.WebAppSettings;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DisplaySettings;
import ch.systemsx.cisd.openbis.generic.shared.dto.PersonPE;
@@ -32,91 +36,169 @@ public class DisplaySettingsProvider
{
// user id to display settings map
private final Map<String, DisplaySettings> displaySettingsMap =
- new HashMap<String, DisplaySettings>();
+ Collections.synchronizedMap(new HashMap<String, DisplaySettings>());
- public synchronized void addDisplaySettingsForPerson(PersonPE person)
+ // user id to lock map
+ private final Map<String, Lock> locksMap = Collections.synchronizedMap(new HashMap<String, Lock>());
+
+ public void addDisplaySettingsForPerson(final PersonPE person)
{
- DisplaySettings settings = displaySettingsMap.get(person.getUserId());
- if (settings == null)
- {
- settings = person.getDisplaySettings();
- displaySettingsMap.put(person.getUserId(), settings);
- }
+ executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
+ {
+ @Override
+ public Void execute(boolean didOperationSucceed)
+ {
+ DisplaySettings settings = displaySettingsMap.get(person.getUserId());
+ if (settings == null)
+ {
+ settings = person.getDisplaySettings();
+ displaySettingsMap.put(person.getUserId(), settings);
+ }
+ return null;
+ }
+ });
}
- public synchronized DisplaySettings getCurrentDisplaySettings(PersonPE person)
+ public DisplaySettings getCurrentDisplaySettings(final PersonPE person)
{
- DisplaySettings settings = displaySettingsMap.get(person.getUserId());
- if (settings == null)
- {
- settings = person.getDisplaySettings();
- displaySettingsMap.put(person.getUserId(), settings);
- }
- settings = new DisplaySettings(settings);
- return settings;
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
+ {
+ @Override
+ public DisplaySettings execute(boolean didOperationSucceed)
+ {
+ DisplaySettings settings = displaySettingsMap.get(person.getUserId());
+ if (settings == null)
+ {
+ settings = person.getDisplaySettings();
+ displaySettingsMap.put(person.getUserId(), settings);
+ }
+ settings = new DisplaySettings(settings);
+ return settings;
+ }
+ });
}
@SuppressWarnings("deprecation")
- public synchronized DisplaySettings getRegularDisplaySettings(PersonPE person)
+ public DisplaySettings getRegularDisplaySettings(final PersonPE person)
{
- DisplaySettings settings = displaySettingsMap.get(person.getUserId());
- if (settings == null)
- {
- settings = person.getDisplaySettings();
- displaySettingsMap.put(person.getUserId(), settings);
- }
- settings = new DisplaySettings(settings);
- settings.clearCustomWebAppSettings();
- return settings;
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
+ {
+ @Override
+ public DisplaySettings execute(boolean didOperationSucceed)
+ {
+ DisplaySettings settings = displaySettingsMap.get(person.getUserId());
+ if (settings == null)
+ {
+ settings = person.getDisplaySettings();
+ displaySettingsMap.put(person.getUserId(), settings);
+ }
+ settings = new DisplaySettings(settings);
+ settings.clearCustomWebAppSettings();
+ return settings;
+ }
+ });
}
- public synchronized DisplaySettings replaceCurrentDisplaySettings(PersonPE person,
- DisplaySettings settings)
+ public DisplaySettings replaceCurrentDisplaySettings(final PersonPE person,
+ final DisplaySettings settings)
{
- displaySettingsMap.put(person.getUserId(), settings);
- person.setDisplaySettings(settings);
- return settings;
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
+ {
+ @Override
+ public DisplaySettings execute(boolean didOperationSucceed)
+ {
+ displaySettingsMap.put(person.getUserId(), settings);
+ person.setDisplaySettings(settings);
+ return settings;
+ }
+ });
}
@SuppressWarnings("deprecation")
- public synchronized DisplaySettings replaceRegularDisplaySettings(PersonPE person,
- DisplaySettings settings)
+ public DisplaySettings replaceRegularDisplaySettings(final PersonPE person,
+ final DisplaySettings settings)
{
- final DisplaySettings oldSettings = displaySettingsMap.get(person.getUserId());
- if (oldSettings != null)
- {
- settings.overwriteCustomWebAppSettings(oldSettings);
- settings.overwriteColumnSettings(oldSettings);
- }
- displaySettingsMap.put(person.getUserId(), settings);
- person.setDisplaySettings(settings);
- return settings;
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
+ {
+ @Override
+ public DisplaySettings execute(boolean didOperationSucceed)
+ {
+ final DisplaySettings oldSettings = displaySettingsMap.get(person.getUserId());
+ if (oldSettings != null)
+ {
+ settings.overwriteCustomWebAppSettings(oldSettings);
+ settings.overwriteColumnSettings(oldSettings);
+ }
+ displaySettingsMap.put(person.getUserId(), settings);
+ person.setDisplaySettings(settings);
+ return settings;
+ }
+ });
}
@SuppressWarnings("deprecation")
- public synchronized WebAppSettings getWebAppSettings(PersonPE person, String webAppId)
+ public WebAppSettings getWebAppSettings(final PersonPE person, final String webAppId)
{
- DisplaySettings settings = displaySettingsMap.get(person.getUserId());
- if (settings == null)
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<WebAppSettings>()
+ {
+ @Override
+ public WebAppSettings execute(boolean didOperationSucceed)
+ {
+ DisplaySettings settings = displaySettingsMap.get(person.getUserId());
+ if (settings == null)
+ {
+ settings = person.getDisplaySettings();
+ displaySettingsMap.put(person.getUserId(), settings);
+ }
+ return new WebAppSettings(webAppId, settings.getCustomWebAppSettings(webAppId));
+ }
+ });
+ }
+
+ @SuppressWarnings("deprecation")
+ public DisplaySettings replaceWebAppSettings(final PersonPE person,
+ final WebAppSettings webAppSettings)
+ {
+ return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
+ {
+ @Override
+ public DisplaySettings execute(boolean didOperationSucceed)
+ {
+ DisplaySettings settings = displaySettingsMap.get(person.getUserId());
+ if (settings == null)
+ {
+ settings = person.getDisplaySettings();
+ displaySettingsMap.put(person.getUserId(), settings);
+ }
+ settings.setCustomWebAppSettings(webAppSettings.getWebAppId(), webAppSettings.getSettings());
+ person.setDisplaySettings(settings);
+ return settings;
+ }
+ });
+ }
+
+ public <T> T executeActionWithPersonLock(PersonPE person, IDelegatedActionWithResult<T> action)
+ {
+ Lock lock = getPersonLock(person);
+ lock.lock();
+ try
+ {
+ return action.execute(true);
+ } finally
{
- settings = person.getDisplaySettings();
- displaySettingsMap.put(person.getUserId(), settings);
+ lock.unlock();
}
- return new WebAppSettings(webAppId, settings.getCustomWebAppSettings(webAppId));
}
- @SuppressWarnings("deprecation")
- public synchronized DisplaySettings replaceWebAppSettings(PersonPE person,
- WebAppSettings webAppSettings)
+ private synchronized Lock getPersonLock(PersonPE person)
{
- DisplaySettings settings = displaySettingsMap.get(person.getUserId());
- if (settings == null)
+ Lock lock = locksMap.get(person.getUserId());
+ if (lock == null)
{
- settings = person.getDisplaySettings();
- displaySettingsMap.put(person.getUserId(), settings);
+ lock = new ReentrantLock();
+ locksMap.put(person.getUserId(), lock);
}
- settings.setCustomWebAppSettings(webAppSettings.getWebAppId(), webAppSettings.getSettings());
- person.setDisplaySettings(settings);
- return settings;
+ return lock;
}
+
}
diff --git a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/CommonServerTest.java b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/CommonServerTest.java
index 5c75c3ff9f0d609c8c68cdde6a2b4438787f214c..7d93413de389614fb94cab089e35f3da600b799a 100644
--- a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/CommonServerTest.java
+++ b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/CommonServerTest.java
@@ -242,14 +242,14 @@ public class CommonServerTest extends SystemTestCase
assertEquals(expected, propertyCodes.toString());
}
- @Test(timeOut = 5000, enabled = false)
+ @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER)
public void testConcurrentDisplaySettingsUpdateForOneUserIsSafe()
{
testConcurrentDisplaySettingsUpdateForUsersIsSafe(new String[] { "test" }, 10, 10);
}
- @Test(timeOut = 5000, enabled = false)
+ @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER)
public void testConcurrentDisplaySettingsUpdateForDifferentUsersIsSafe()
{
@@ -269,9 +269,11 @@ public class CommonServerTest extends SystemTestCase
for (int u = 0; u < users.length; u++)
{
+
for (int i = 0; i < numberOfThreads; i++)
{
sessionContext[u] = commonServer.tryAuthenticate(users[u], PASSWORD);
+ new SetPanelSizeRunnable(commonServer, sessionContext[u].getSessionToken(), PANEL_ID, 0).run();
IncrementPanelSizeRunnable runnable =
new IncrementPanelSizeRunnable(commonServer, sessionContext[u].getSessionToken(), PANEL_ID, numberOfIterations);
runnable.setSendChannel(sendChannel);
@@ -300,7 +302,7 @@ public class CommonServerTest extends SystemTestCase
}
}
- @Test(timeOut = 5000, enabled = false)
+ @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER)
public void testLongRunninngDisplaySettingsUpdateForOneUserBlocksOtherUpdatesForThisUser() throws Exception
{
@@ -341,6 +343,7 @@ public class CommonServerTest extends SystemTestCase
/*
* Will concurrently update the same person in two separate transactions.
*/
+
IncrementPanelSizeRunnable runnable1 = new IncrementPanelSizeRunnable(commonServer, sessionContext1.getSessionToken(), PANEL_ID, 1);
IncrementPanelSizeRunnable runnable2 = new IncrementPanelSizeRunnable(commonServer, sessionContext2.getSessionToken(), PANEL_ID, 1);
@@ -388,7 +391,7 @@ public class CommonServerTest extends SystemTestCase
}
}
- @Test(timeOut = 5000, enabled = false)
+ @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER)
public void testLongRunninngDisplaySettingsUpdateForOneUserDoesNotBlockUpdatesForOtherUsers() throws Exception
{
@@ -486,6 +489,45 @@ public class CommonServerTest extends SystemTestCase
}
}
+ private static class SetPanelSizeRunnable implements Runnable
+ {
+ private ICommonServer server;
+
+ private String sessionToken;
+
+ private String panelId;
+
+ private int value;
+
+ public SetPanelSizeRunnable(ICommonServer server, String sessionToken, String panelId, int value)
+ {
+ this.server = server;
+ this.sessionToken = sessionToken;
+ this.panelId = panelId;
+ this.value = value;
+ }
+
+ @Override
+ public void run()
+ {
+ IDisplaySettingsUpdate update = new IDisplaySettingsUpdate()
+ {
+
+ private static final long serialVersionUID = 1L;
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public DisplaySettings update(DisplaySettings displaySettings)
+ {
+ Map<String, Integer> panelSizeSettings = displaySettings.getPanelSizeSettings();
+ panelSizeSettings.put(panelId, value);
+ return displaySettings;
+ }
+ };
+ server.updateDisplaySettings(sessionToken, update);
+ }
+ }
+
private static class IncrementPanelSizeRunnable implements Runnable
{
private ICommonServer server;