From c76dc463b8ca8a78177cc3eef4be2981a6a1a5ce Mon Sep 17 00:00:00 2001
From: pkupczyk <piotr.kupczyk@id.ethz.ch>
Date: Tue, 19 Dec 2023 14:32:23 +0100
Subject: [PATCH] SSDM-14263 : Deadlock on display settings
---
.../generic/server/AbstractServer.java | 94 ++++++++++---------
.../server/DisplaySettingsProvider.java | 16 ++--
.../v1/GeneralInformationChangingService.java | 15 ++-
...GeneralInformationChangingServiceTest.java | 47 ++++++++++
4 files changed, 118 insertions(+), 54 deletions(-)
diff --git a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
index 1b6c08dd377..cb5e1d90108 100644
--- a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
+++ b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/AbstractServer.java
@@ -620,55 +620,63 @@ public abstract class AbstractServer<T> extends AbstractServiceWithLogger<T> imp
}
final Session session = sessionManager.getSession(sessionToken);
- List<PersonPE> persons = null;
- PersonPE person = daoFactory.getPersonDAO().tryFindPersonByUserId(session.getUserName());
- final Set<RoleAssignmentPE> roles;
- if (person == null)
- {
- persons = daoFactory.getPersonDAO().listPersons();
- final PersonPE systemUser = getSystemUser(persons);
- final DisplaySettings defaultDisplaySettings = getDefaultDisplaySettings(sessionToken);
- person = createPerson(session.getPrincipal(), systemUser, defaultDisplaySettings);
- roles = Collections.emptySet();
- } else
- {
- updatePersonIfNecessary(person, session.getPrincipal());
- roles = person.getAllPersonRoles();
- HibernateUtils.initialize(roles);
- }
- if (session.tryGetPerson() == null)
- {
- session.setPerson(person);
- session.setCreatorPerson(person);
- displaySettingsProvider.addDisplaySettingsForPerson(person);
- }
- if (roles.isEmpty())
+ return displaySettingsProvider.executeActionWithPersonLock(session.getUserName(), new IDelegatedActionWithResult<SessionContextDTO>()
{
- if (persons == null)
- {
- persons = daoFactory.getPersonDAO().listPersons();
- }
- if (isFirstLoggedUser(person, persons))
- {
- grantRoleAtFirstLogin(persons, person, RoleCode.ADMIN);
- } else if (isFirstLoggedETLServer(person, persons))
+ @Override public SessionContextDTO execute(final boolean didOperationSucceed)
{
- grantRoleAtFirstLogin(persons, person, RoleCode.ETL_SERVER);
- } else
- {
- throw createException(person, "has no role assignments");
- }
- }
+ List<PersonPE> persons = null;
+ PersonPE person = daoFactory.getPersonDAO().tryFindPersonByUserId(session.getUserName());
+ final Set<RoleAssignmentPE> roles;
- if (false == person.isActive())
- {
- throw createException(person, "has been deactivated");
- }
+ if (person == null)
+ {
+ persons = daoFactory.getPersonDAO().listPersons();
+ final PersonPE systemUser = getSystemUser(persons);
+ final DisplaySettings defaultDisplaySettings = getDefaultDisplaySettings(sessionToken);
+ person = createPerson(session.getPrincipal(), systemUser, defaultDisplaySettings);
+ roles = Collections.emptySet();
+ } else
+ {
+ updatePersonIfNecessary(person, session.getPrincipal());
+ roles = person.getAllPersonRoles();
+ HibernateUtils.initialize(roles);
+ }
+ if (session.tryGetPerson() == null)
+ {
+ session.setPerson(person);
+ session.setCreatorPerson(person);
+ displaySettingsProvider.addDisplaySettingsForPerson(person);
+ }
+
+ if (roles.isEmpty())
+ {
+ if (persons == null)
+ {
+ persons = daoFactory.getPersonDAO().listPersons();
+ }
+ if (isFirstLoggedUser(person, persons))
+ {
+ grantRoleAtFirstLogin(persons, person, RoleCode.ADMIN);
+ } else if (isFirstLoggedETLServer(person, persons))
+ {
+ grantRoleAtFirstLogin(persons, person, RoleCode.ETL_SERVER);
+ } else
+ {
+ throw createException(person, "has no role assignments");
+ }
+ }
- removeNotExistingVisits(session);
+ if (false == person.isActive())
+ {
+ throw createException(person, "has been deactivated");
+ }
- return asDTO(session);
+ removeNotExistingVisits(session);
+
+ return asDTO(session);
+ }
+ });
}
private UserFailureException createException(PersonPE person, String reason)
diff --git a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
index 6802b2c9dc9..418a53efc17 100644
--- a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
+++ b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/DisplaySettingsProvider.java
@@ -195,9 +195,8 @@ public class DisplaySettingsProvider
});
}
- public <T> T executeActionWithPersonLock(PersonPE person, IDelegatedActionWithResult<T> action)
- {
- Lock lock = getPersonLock(person);
+ public <T> T executeActionWithPersonLock(String userId, IDelegatedActionWithResult<T> action){
+ Lock lock = getPersonLock(userId);
lock.lock();
try
{
@@ -208,13 +207,18 @@ public class DisplaySettingsProvider
}
}
- private synchronized Lock getPersonLock(PersonPE person)
+ public <T> T executeActionWithPersonLock(PersonPE person, IDelegatedActionWithResult<T> action)
+ {
+ return executeActionWithPersonLock(person.getUserId(), action);
+ }
+
+ private synchronized Lock getPersonLock(String userId)
{
- Lock lock = locksMap.get(person.getUserId());
+ Lock lock = locksMap.get(userId);
if (lock == null)
{
lock = new ReentrantLock();
- locksMap.put(person.getUserId(), lock);
+ locksMap.put(userId, lock);
}
return lock;
}
diff --git a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/api/v1/GeneralInformationChangingService.java b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/api/v1/GeneralInformationChangingService.java
index 9f42038e418..c724f7d56b9 100644
--- a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/api/v1/GeneralInformationChangingService.java
+++ b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/api/v1/GeneralInformationChangingService.java
@@ -24,6 +24,7 @@ import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;
import ch.rinn.restrictions.Private;
+import ch.systemsx.cisd.common.action.IDelegatedActionWithResult;
import ch.systemsx.cisd.common.exceptions.InvalidSessionException;
import ch.systemsx.cisd.common.servlet.IRequestContextProvider;
import ch.systemsx.cisd.openbis.common.spring.IInvocationLoggerContext;
@@ -146,14 +147,18 @@ public class GeneralInformationChangingService extends
try
{
final Session session = getSession(sessionToken);
- PersonPE person = session.tryGetPerson();
+ final PersonPE person = session.tryGetPerson();
if (person != null)
{
- synchronized (displaySettingsProvider)
+ displaySettingsProvider.executeActionWithPersonLock(person, new IDelegatedActionWithResult<Object>()
{
- displaySettingsProvider.replaceWebAppSettings(person, webAppSettings);
- getDAOFactory().getPersonDAO().updatePerson(person);
- }
+ @Override public Object execute(final boolean didOperationSucceed)
+ {
+ displaySettingsProvider.replaceWebAppSettings(person, webAppSettings);
+ getDAOFactory().getPersonDAO().updatePerson(person);
+ return null;
+ }
+ });
}
} catch (InvalidSessionException e)
{
diff --git a/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/api/v1/GeneralInformationChangingServiceTest.java b/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/api/v1/GeneralInformationChangingServiceTest.java
index 22e39cdb363..25001c18358 100644
--- a/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/api/v1/GeneralInformationChangingServiceTest.java
+++ b/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/api/v1/GeneralInformationChangingServiceTest.java
@@ -26,6 +26,7 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
+import java.util.UUID;
import org.hibernate.Session;
import org.springframework.beans.factory.annotation.Autowired;
@@ -53,6 +54,7 @@ import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.SearchCriteria;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.SearchCriteria.MatchClause;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.SearchCriteria.MatchClauseAttribute;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.Vocabulary;
+import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.WebAppSettings;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.id.dataset.DataSetCodeId;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.id.experiment.ExperimentIdentifierId;
import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.id.material.MaterialCodeAndTypeCodeId;
@@ -68,6 +70,7 @@ import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewExperiment;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.SampleParentWithDerived;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.VocabularyTerm;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.VocabularyTermReplacement;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.WebApp;
import ch.systemsx.cisd.openbis.generic.shared.dto.ExperimentPE;
import ch.systemsx.cisd.openbis.generic.shared.dto.MetaprojectAssignmentPE;
import ch.systemsx.cisd.openbis.generic.shared.dto.MetaprojectPE;
@@ -1066,6 +1069,50 @@ public class GeneralInformationChangingServiceTest extends SystemTestCase
}
}
+ @Test
+ public void testSetWebAppSettings()
+ {
+ String webAppId1 = UUID.randomUUID().toString();
+ String webAppId2 = UUID.randomUUID().toString();
+ String adminSessionToken = generalInformationService.tryToAuthenticateForAllServices(TEST_USER, PASSWORD);
+
+ // get empty settings 1 and 2
+
+ WebAppSettings settings1 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId1);
+ assertEquals(webAppId1, settings1.getWebAppId());
+ assertEquals(Collections.emptyMap(), settings1.getSettings());
+
+ WebAppSettings settings2 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId2);
+ assertEquals(webAppId2, settings2.getWebAppId());
+ assertEquals(Collections.emptyMap(), settings2.getSettings());
+
+ // set settings 1
+
+ WebAppSettings settings1Creation = new WebAppSettings(webAppId1, Collections.singletonMap("test-key-1", "test-value-1"));
+ generalInformationChangingService.setWebAppSettings(adminSessionToken, settings1Creation);
+
+ settings1 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId1);
+ assertEquals(webAppId1, settings1.getWebAppId());
+ assertEquals(settings1Creation.getSettings(), settings1.getSettings());
+
+ settings2 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId2);
+ assertEquals(webAppId2, settings2.getWebAppId());
+ assertEquals(Collections.emptyMap(), settings2.getSettings());
+
+ // set settings 2
+
+ WebAppSettings settings2Creation = new WebAppSettings(webAppId2, Collections.singletonMap("test-key-2", "test-value-2"));
+ generalInformationChangingService.setWebAppSettings(adminSessionToken, settings2Creation);
+
+ settings1 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId1);
+ assertEquals(webAppId1, settings1.getWebAppId());
+ assertEquals(settings1Creation.getSettings(), settings1.getSettings());
+
+ settings2 = generalInformationChangingService.getWebAppSettings(adminSessionToken, webAppId2);
+ assertEquals(webAppId2, settings2.getWebAppId());
+ assertEquals(settings2.getSettings(), settings2.getSettings());
+ }
+
private class ProjectDeleteAction implements DeleteAction<Project>
{
@Override
--
GitLab