From 6e94931023995a6aa0c088c7bfdc85a86ea6d598 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 11 Jul 2016 14:41:12 +0200 Subject: [PATCH] KEYCLOAK-3296 same user logging twice at the same time causes lock issue - HQL deletion patch --- .../org/keycloak/models/jpa/UserAdapter.java | 14 ++- .../jpa/entities/UserAttributeEntity.java | 1 + .../ldap/base/LDAPMultipleAttributesTest.java | 8 ++ .../model/ConcurrentTransactionsTest.java | 97 +++++++++++++++++-- 4 files changed, 102 insertions(+), 18 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java index 5d51fc939e..2ea12fd839 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java @@ -47,6 +47,7 @@ import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.common.util.Time; import javax.persistence.EntityManager; +import javax.persistence.Query; import javax.persistence.TypedQuery; import java.util.ArrayList; @@ -172,14 +173,11 @@ public class UserAdapter implements UserModel, JpaModel { @Override public void removeAttribute(String name) { - Iterator it = user.getAttributes().iterator(); - while (it.hasNext()) { - UserAttributeEntity attr = it.next(); - if (attr.getName().equals(name)) { - it.remove(); - em.remove(attr); - } - } + // KEYCLOAK-3296 : Remove attribute through HQL to avoid StaleUpdateException + Query query = em.createNamedQuery("deleteUserAttributesByNameAndUser"); + query.setParameter("name", name); + query.setParameter("userId", user.getId()); + int numUpdated = query.executeUpdate(); } @Override diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserAttributeEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserAttributeEntity.java index 263757fc54..2b5d35cc63 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserAttributeEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserAttributeEntity.java @@ -42,6 +42,7 @@ import java.util.Set; @NamedQueries({ @NamedQuery(name="getAttributesByNameAndValue", query="select attr from UserAttributeEntity attr where attr.name = :name and attr.value = :value"), @NamedQuery(name="deleteUserAttributesByRealm", query="delete from UserAttributeEntity attr where attr.user IN (select u from UserEntity u where u.realmId=:realmId)"), + @NamedQuery(name="deleteUserAttributesByNameAndUser", query="delete from UserAttributeEntity attr where attr.user.id = :userId and attr.name = :name"), @NamedQuery(name="deleteUserAttributesByRealmAndLink", query="delete from UserAttributeEntity attr where attr.user IN (select u from UserEntity u where u.realmId=:realmId and u.federationLink=:link)") }) @Table(name="USER_ATTRIBUTE") diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPMultipleAttributesTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPMultipleAttributesTest.java index 2e6a10a8d5..fb23b5dd50 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPMultipleAttributesTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPMultipleAttributesTest.java @@ -166,6 +166,14 @@ public class LDAPMultipleAttributesTest { postalCodes.add("77332"); user.setAttribute("postal_code", postalCodes); + } finally { + keycloakRule.stopSession(session, true); + } + + session = keycloakRule.startSession(); + try { + RealmModel appRealm = session.realms().getRealmByName("test"); + UserModel user = session.users().getUserByUsername("bwilson", appRealm); assertPostalCodes(user.getAttribute("postal_code"), "88441", "77332"); } finally { keycloakRule.stopSession(session, true); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/ConcurrentTransactionsTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/ConcurrentTransactionsTest.java index 93b6458d8d..b3ad0c4b5b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/ConcurrentTransactionsTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/ConcurrentTransactionsTest.java @@ -18,7 +18,9 @@ package org.keycloak.testsuite.model; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; +import org.jboss.logging.Logger; import org.junit.Assert; import org.junit.Ignore; import org.junit.Test; @@ -28,6 +30,7 @@ import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.KeycloakSessionTask; import org.keycloak.models.RealmModel; import org.keycloak.models.RealmProvider; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; /** @@ -35,6 +38,8 @@ import org.keycloak.models.utils.KeycloakModelUtils; */ public class ConcurrentTransactionsTest extends AbstractModelTest { + private static final Logger logger = Logger.getLogger(ConcurrentTransactionsTest.class); + @Test public void persistClient() throws Exception { RealmModel realm = realmManager.createRealm("original"); @@ -63,21 +68,21 @@ public class ConcurrentTransactionsTest extends AbstractModelTest { try { // Wait until transaction in both threads started transactionsCounter.countDown(); - System.out.println("transaction1 started"); + logger.info("transaction1 started"); transactionsCounter.await(); // Read client RealmModel realm = session.realms().getRealmByName("original"); ClientModel client = session.realms().getClientByClientId("client", realm); - System.out.println("transaction1: Read client finished"); + logger.info("transaction1: Read client finished"); readLatch.countDown(); // Wait until thread2 updates client and commits updateLatch.await(); - System.out.println("transaction1: Going to read client again"); + logger.info("transaction1: Going to read client again"); client = session.realms().getClientByClientId("client", realm); - System.out.println("transaction1: secret: " + client.getSecret()); + logger.info("transaction1: secret: " + client.getSecret()); } catch (Exception e) { throw new RuntimeException(e); } @@ -99,12 +104,12 @@ public class ConcurrentTransactionsTest extends AbstractModelTest { try { // Wait until transaction in both threads started transactionsCounter.countDown(); - System.out.println("transaction2 started"); + logger.info("transaction2 started"); transactionsCounter.await(); readLatch.await(); - System.out.println("transaction2: Going to update client secret"); + logger.info("transaction2: Going to update client secret"); RealmModel realm = session.realms().getRealmByName("original"); ClientModel client = session.realms().getClientByClientId("client", realm); @@ -116,7 +121,7 @@ public class ConcurrentTransactionsTest extends AbstractModelTest { }); - System.out.println("transaction2: commited"); + logger.info("transaction2: commited"); updateLatch.countDown(); } @@ -128,7 +133,7 @@ public class ConcurrentTransactionsTest extends AbstractModelTest { thread1.join(); thread2.join(); - System.out.println("after thread join"); + logger.info("after thread join"); commit(); @@ -138,11 +143,83 @@ public class ConcurrentTransactionsTest extends AbstractModelTest { ClientModel clientFromCache = session.realms().getClientById(clientDBId, realm); ClientModel clientFromDB = session.getProvider(RealmProvider.class).getClientById(clientDBId, realm); - System.out.println("SECRET FROM DB : " + clientFromDB.getSecret()); - System.out.println("SECRET FROM CACHE : " + clientFromCache.getSecret()); + logger.info("SECRET FROM DB : " + clientFromDB.getSecret()); + logger.info("SECRET FROM CACHE : " + clientFromCache.getSecret()); Assert.assertEquals("new", clientFromDB.getSecret()); Assert.assertEquals("new", clientFromCache.getSecret()); } + + // KEYCLOAK-3296 + @Test + public void removeUserAttribute() throws Exception { + RealmModel realm = realmManager.createRealm("original"); + KeycloakSession session = realmManager.getSession(); + + UserModel user = session.users().addUser(realm, "john"); + user.setSingleAttribute("foo", "val1"); + + final KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory(); + commit(); + + AtomicReference reference = new AtomicReference<>(); + + final CountDownLatch readAttrLatch = new CountDownLatch(2); + + Runnable runnable = new Runnable() { + + @Override + public void run() { + try { + KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() { + + @Override + public void run(KeycloakSession session) { + try { + // Read user attribute + RealmModel realm = session.realms().getRealmByName("original"); + UserModel john = session.users().getUserByUsername("john", realm); + String attrVal = john.getFirstAttribute("foo"); + + // Wait until it's read in both threads + readAttrLatch.countDown(); + readAttrLatch.await(); + + // Remove user attribute in both threads + john.removeAttribute("foo"); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + }); + } catch (Exception e) { + reference.set(e); + throw new RuntimeException(e); + } finally { + readAttrLatch.countDown(); + } + } + + }; + + Thread thread1 = new Thread(runnable); + Thread thread2 = new Thread(runnable); + + thread1.start(); + thread2.start(); + + thread1.join(); + thread2.join(); + + logger.info("removeUserAttribute: after thread join"); + + commit(); + + if (reference.get() != null) { + Assert.fail("Exception happened in some of threads. Details: " + reference.get().getMessage()); + } + } + }