Skip to content
Snippets Groups Projects
Commit 649cbf84 authored by pkupczyk's avatar pkupczyk
Browse files

SP-835 / BIS-523 : PhosphonetX server hangs from time to time - fix 

SVN: 29709
parent 0f8f28e1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
openbis/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
+ 37
− 26
View file @ 649cbf84
...@@ -40,6 +40,7 @@ import org.springframework.transaction.annotation.Transactional; ...@@ -40,6 +40,7 @@ import org.springframework.transaction.annotation.Transactional;
import ch.systemsx.cisd.authentication.IPrincipalProvider; import ch.systemsx.cisd.authentication.IPrincipalProvider;
import ch.systemsx.cisd.authentication.ISessionManager; import ch.systemsx.cisd.authentication.ISessionManager;
import ch.systemsx.cisd.authentication.Principal; 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.InvalidSessionException;
import ch.systemsx.cisd.common.exceptions.UserFailureException; import ch.systemsx.cisd.common.exceptions.UserFailureException;
import ch.systemsx.cisd.common.mail.IMailClient; import ch.systemsx.cisd.common.mail.IMailClient;
...@@ -753,31 +754,36 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp ...@@ -753,31 +754,36 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
@SuppressWarnings("deprecation") @SuppressWarnings("deprecation")
@Override @Override
public void saveDisplaySettings(String sessionToken, DisplaySettings displaySettings, public void saveDisplaySettings(String sessionToken, final DisplaySettings displaySettings,
int maxEntityVisits) final int maxEntityVisits)
{ {
try try
{ {
final Session session = getSession(sessionToken); final Session session = getSession(sessionToken);
synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions() synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions()
{ {
PersonPE person = session.tryGetPerson(); final PersonPE person = session.tryGetPerson();
if (person != null) if (person != null)
{ {
synchronized (displaySettingsProvider) displaySettingsProvider.executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
{
if (maxEntityVisits >= 0)
{ {
List<EntityVisit> visits = displaySettings.getVisits(); @Override
sortAndRemoveMultipleVisits(visits); public Void execute(boolean didOperationSucceed)
for (int i = visits.size() - 1; i >= maxEntityVisits; i--)
{ {
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) } catch (InvalidSessionException e)
...@@ -788,7 +794,7 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp ...@@ -788,7 +794,7 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
@Override @Override
public void updateDisplaySettings(String sessionToken, public void updateDisplaySettings(String sessionToken,
IDisplaySettingsUpdate displaySettingsUpdate) final IDisplaySettingsUpdate displaySettingsUpdate)
{ {
if (displaySettingsUpdate == null) if (displaySettingsUpdate == null)
{ {
...@@ -800,19 +806,24 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp ...@@ -800,19 +806,24 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
final Session session = getSession(sessionToken); final Session session = getSession(sessionToken);
synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions() synchronized (session) // synchronized with OpenBisSessionManager.updateAllSessions()
{ {
PersonPE person = session.tryGetPerson(); final PersonPE person = session.tryGetPerson();
if (person != null) if (person != null)
{ {
synchronized (displaySettingsProvider) displaySettingsProvider.executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
{ {
DisplaySettings currentDisplaySettings = @Override
displaySettingsProvider.getCurrentDisplaySettings(person); public Void execute(boolean didOperationSucceed)
DisplaySettings newDisplaySettings = {
displaySettingsUpdate.update(currentDisplaySettings); DisplaySettings currentDisplaySettings =
displaySettingsProvider.replaceCurrentDisplaySettings(person, displaySettingsProvider.getCurrentDisplaySettings(person);
newDisplaySettings); DisplaySettings newDisplaySettings =
getDAOFactory().getPersonDAO().updatePerson(person); displaySettingsUpdate.update(currentDisplaySettings);
} displaySettingsProvider.replaceCurrentDisplaySettings(person,
newDisplaySettings);
getDAOFactory().getPersonDAO().updatePerson(person);
return null;
}
});
} }
} }
} catch (InvalidSessionException e) } catch (InvalidSessionException e)
......
openbis/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
+ 141
− 59
View file @ 649cbf84
...@@ -16,9 +16,13 @@ ...@@ -16,9 +16,13 @@
package ch.systemsx.cisd.openbis.generic.server; package ch.systemsx.cisd.openbis.generic.server;
import java.util.Collections;
import java.util.HashMap; import java.util.HashMap;
import java.util.Map; 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.api.v1.dto.WebAppSettings;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DisplaySettings; import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DisplaySettings;
import ch.systemsx.cisd.openbis.generic.shared.dto.PersonPE; import ch.systemsx.cisd.openbis.generic.shared.dto.PersonPE;
...@@ -32,91 +36,169 @@ public class DisplaySettingsProvider ...@@ -32,91 +36,169 @@ public class DisplaySettingsProvider
{ {
// user id to display settings map // user id to display settings map
private final Map<String, DisplaySettings> displaySettingsMap = 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()); executeActionWithPersonLock(person, new IDelegatedActionWithResult<Void>()
if (settings == null) {
{ @Override
settings = person.getDisplaySettings(); public Void execute(boolean didOperationSucceed)
displaySettingsMap.put(person.getUserId(), settings); {
} 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()); return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
if (settings == null) {
{ @Override
settings = person.getDisplaySettings(); public DisplaySettings execute(boolean didOperationSucceed)
displaySettingsMap.put(person.getUserId(), settings); {
} DisplaySettings settings = displaySettingsMap.get(person.getUserId());
settings = new DisplaySettings(settings); if (settings == null)
return settings; {
settings = person.getDisplaySettings();
displaySettingsMap.put(person.getUserId(), settings);
}
settings = new DisplaySettings(settings);
return settings;
}
});
} }
@SuppressWarnings("deprecation") @SuppressWarnings("deprecation")
public synchronized DisplaySettings getRegularDisplaySettings(PersonPE person) public DisplaySettings getRegularDisplaySettings(final PersonPE person)
{ {
DisplaySettings settings = displaySettingsMap.get(person.getUserId()); return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
if (settings == null) {
{ @Override
settings = person.getDisplaySettings(); public DisplaySettings execute(boolean didOperationSucceed)
displaySettingsMap.put(person.getUserId(), settings); {
} DisplaySettings settings = displaySettingsMap.get(person.getUserId());
settings = new DisplaySettings(settings); if (settings == null)
settings.clearCustomWebAppSettings(); {
return settings; settings = person.getDisplaySettings();
displaySettingsMap.put(person.getUserId(), settings);
}
settings = new DisplaySettings(settings);
settings.clearCustomWebAppSettings();
return settings;
}
});
} }
public synchronized DisplaySettings replaceCurrentDisplaySettings(PersonPE person, public DisplaySettings replaceCurrentDisplaySettings(final PersonPE person,
DisplaySettings settings) final DisplaySettings settings)
{ {
displaySettingsMap.put(person.getUserId(), settings); return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
person.setDisplaySettings(settings); {
return settings; @Override
public DisplaySettings execute(boolean didOperationSucceed)
{
displaySettingsMap.put(person.getUserId(), settings);
person.setDisplaySettings(settings);
return settings;
}
});
} }
@SuppressWarnings("deprecation") @SuppressWarnings("deprecation")
public synchronized DisplaySettings replaceRegularDisplaySettings(PersonPE person, public DisplaySettings replaceRegularDisplaySettings(final PersonPE person,
DisplaySettings settings) final DisplaySettings settings)
{ {
final DisplaySettings oldSettings = displaySettingsMap.get(person.getUserId()); return executeActionWithPersonLock(person, new IDelegatedActionWithResult<DisplaySettings>()
if (oldSettings != null) {
{ @Override
settings.overwriteCustomWebAppSettings(oldSettings); public DisplaySettings execute(boolean didOperationSucceed)
settings.overwriteColumnSettings(oldSettings); {
} final DisplaySettings oldSettings = displaySettingsMap.get(person.getUserId());
displaySettingsMap.put(person.getUserId(), settings); if (oldSettings != null)
person.setDisplaySettings(settings); {
return settings; settings.overwriteCustomWebAppSettings(oldSettings);
settings.overwriteColumnSettings(oldSettings);
}
displaySettingsMap.put(person.getUserId(), settings);
person.setDisplaySettings(settings);
return settings;
}
});
} }
@SuppressWarnings("deprecation") @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()); return executeActionWithPersonLock(person, new IDelegatedActionWithResult<WebAppSettings>()
if (settings == null) {
@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(); lock.unlock();
displaySettingsMap.put(person.getUserId(), settings);
} }
return new WebAppSettings(webAppId, settings.getCustomWebAppSettings(webAppId));
} }
@SuppressWarnings("deprecation") private synchronized Lock getPersonLock(PersonPE person)
public synchronized DisplaySettings replaceWebAppSettings(PersonPE person,
WebAppSettings webAppSettings)
{ {
DisplaySettings settings = displaySettingsMap.get(person.getUserId()); Lock lock = locksMap.get(person.getUserId());
if (settings == null) if (lock == null)
{ {
settings = person.getDisplaySettings(); lock = new ReentrantLock();
displaySettingsMap.put(person.getUserId(), settings); locksMap.put(person.getUserId(), lock);
} }
settings.setCustomWebAppSettings(webAppSettings.getWebAppId(), webAppSettings.getSettings()); return lock;
person.setDisplaySettings(settings);
return settings;
} }
} }
openbis/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/CommonServerTest.java
+ 46
− 4
View file @ 649cbf84
...@@ -242,14 +242,14 @@ public class CommonServerTest extends SystemTestCase ...@@ -242,14 +242,14 @@ public class CommonServerTest extends SystemTestCase
assertEquals(expected, propertyCodes.toString()); assertEquals(expected, propertyCodes.toString());
} }
@Test(timeOut = 5000, enabled = false) @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER) @Transactional(propagation = Propagation.NEVER)
public void testConcurrentDisplaySettingsUpdateForOneUserIsSafe() public void testConcurrentDisplaySettingsUpdateForOneUserIsSafe()
{ {
testConcurrentDisplaySettingsUpdateForUsersIsSafe(new String[] { "test" }, 10, 10); testConcurrentDisplaySettingsUpdateForUsersIsSafe(new String[] { "test" }, 10, 10);
} }
@Test(timeOut = 5000, enabled = false) @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER) @Transactional(propagation = Propagation.NEVER)
public void testConcurrentDisplaySettingsUpdateForDifferentUsersIsSafe() public void testConcurrentDisplaySettingsUpdateForDifferentUsersIsSafe()
{ {
...@@ -269,9 +269,11 @@ public class CommonServerTest extends SystemTestCase ...@@ -269,9 +269,11 @@ public class CommonServerTest extends SystemTestCase
for (int u = 0; u < users.length; u++) for (int u = 0; u < users.length; u++)
{ {
for (int i = 0; i < numberOfThreads; i++) for (int i = 0; i < numberOfThreads; i++)
{ {
sessionContext[u] = commonServer.tryAuthenticate(users[u], PASSWORD); sessionContext[u] = commonServer.tryAuthenticate(users[u], PASSWORD);
new SetPanelSizeRunnable(commonServer, sessionContext[u].getSessionToken(), PANEL_ID, 0).run();
IncrementPanelSizeRunnable runnable = IncrementPanelSizeRunnable runnable =
new IncrementPanelSizeRunnable(commonServer, sessionContext[u].getSessionToken(), PANEL_ID, numberOfIterations); new IncrementPanelSizeRunnable(commonServer, sessionContext[u].getSessionToken(), PANEL_ID, numberOfIterations);
runnable.setSendChannel(sendChannel); runnable.setSendChannel(sendChannel);
...@@ -300,7 +302,7 @@ public class CommonServerTest extends SystemTestCase ...@@ -300,7 +302,7 @@ public class CommonServerTest extends SystemTestCase
} }
} }
@Test(timeOut = 5000, enabled = false) @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER) @Transactional(propagation = Propagation.NEVER)
public void testLongRunninngDisplaySettingsUpdateForOneUserBlocksOtherUpdatesForThisUser() throws Exception public void testLongRunninngDisplaySettingsUpdateForOneUserBlocksOtherUpdatesForThisUser() throws Exception
{ {
...@@ -341,6 +343,7 @@ public class CommonServerTest extends SystemTestCase ...@@ -341,6 +343,7 @@ public class CommonServerTest extends SystemTestCase
/* /*
* Will concurrently update the same person in two separate transactions. * Will concurrently update the same person in two separate transactions.
*/ */
IncrementPanelSizeRunnable runnable1 = new IncrementPanelSizeRunnable(commonServer, sessionContext1.getSessionToken(), PANEL_ID, 1); IncrementPanelSizeRunnable runnable1 = new IncrementPanelSizeRunnable(commonServer, sessionContext1.getSessionToken(), PANEL_ID, 1);
IncrementPanelSizeRunnable runnable2 = new IncrementPanelSizeRunnable(commonServer, sessionContext2.getSessionToken(), PANEL_ID, 1); IncrementPanelSizeRunnable runnable2 = new IncrementPanelSizeRunnable(commonServer, sessionContext2.getSessionToken(), PANEL_ID, 1);
...@@ -388,7 +391,7 @@ public class CommonServerTest extends SystemTestCase ...@@ -388,7 +391,7 @@ public class CommonServerTest extends SystemTestCase
} }
} }
@Test(timeOut = 5000, enabled = false) @Test(timeOut = 5000)
@Transactional(propagation = Propagation.NEVER) @Transactional(propagation = Propagation.NEVER)
public void testLongRunninngDisplaySettingsUpdateForOneUserDoesNotBlockUpdatesForOtherUsers() throws Exception public void testLongRunninngDisplaySettingsUpdateForOneUserDoesNotBlockUpdatesForOtherUsers() throws Exception
{ {
...@@ -486,6 +489,45 @@ public class CommonServerTest extends SystemTestCase ...@@ -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 static class IncrementPanelSizeRunnable implements Runnable
{ {
private ICommonServer server; private ICommonServer server;
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment