Skip to content

[SYNCOPE-1635] Use singleton bean for each domain for each rule with conf #264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ilgrosso merged 1 commit into from
May 21, 2021
Merged

[SYNCOPE-1635] Use singleton bean for each domain for each rule with conf #264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 20 additions & 19 deletions core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@
*/
package org.apache.syncope.core.spring;

import groovy.lang.GroovyClassLoader;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import groovy.lang.GroovyClassLoader;
import org.apache.syncope.common.lib.policy.AccountRuleConf;
import org.apache.syncope.common.lib.policy.PasswordRuleConf;
import org.apache.syncope.common.lib.policy.PullCorrelationRuleConf;
Expand All @@ -31,14 +31,16 @@
import org.apache.syncope.core.persistence.api.ImplementationLookup;
import org.apache.syncope.core.persistence.api.dao.AccountRule;
import org.apache.syncope.core.persistence.api.dao.PasswordRule;
import org.apache.syncope.core.persistence.api.dao.Reportlet;
import org.apache.syncope.core.persistence.api.entity.Implementation;
import org.apache.syncope.core.persistence.api.dao.PullCorrelationRule;
import org.apache.syncope.core.persistence.api.dao.PushCorrelationRule;
import org.apache.syncope.core.persistence.api.dao.Reportlet;
import org.apache.syncope.core.persistence.api.entity.Implementation;
import org.apache.syncope.core.provisioning.api.serialization.POJOHelper;
import org.apache.syncope.core.spring.security.AuthContextUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.support.AbstractBeanDefinition;
import org.springframework.beans.factory.support.DefaultListableBeanFactory;

public final class ImplementationManager {

Expand Down Expand Up @@ -217,27 +219,26 @@ private static <T> T buildJava(final Implementation impl)

@SuppressWarnings("unchecked")
private static <T> T buildJavaWithConf(final Class<T> clazz) {
T bean = null;

if (clazz != null) {
if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
} else {
try {
bean = (T) ApplicationContextProvider.getBeanFactory().
createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
} catch (IllegalStateException e) {
LOG.debug("While attempting to register {}", clazz.getName(), e);
String domainableBeanNameWithConf = AuthContextUtils.getDomain() + clazz.getName();
DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();

if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {

@ilgrosso ilgrosso May 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same exact if with statement is also performed in the synchronized block: why?

@DmitriyBrashevets DmitriyBrashevets May 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a double-check lock pattern. Thread takes monitor and executes the logic regarding initialization. Another thread waits until the the monitor is realeased. As soon as first thread releases monitor -> second thread proceeds its work and reaches the additional if clause. As the bean already exists (created by first thread) the second thread is not creating a bean again and not registers it as a singleton one more time.

Do you see any disadvantages it this?

@ilgrosso ilgrosso May 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, there is wide literature abut double-check lock pattern, and some is not very pleasant with it, especially for Java implementations.

What would be the disadvantage of simply removing the first if and leaving the whole logic in the synchronized block?

@DmitriyBrashevets DmitriyBrashevets May 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO: Adding of the additional if clause gives a small improvement in performance (because the acquiring of monitor is not requested).

Am I wrong?

@ilgrosso ilgrosso May 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not completely convinced, but let that be :-)

return (T) beanFactory.getSingleton(domainableBeanNameWithConf);
}

// if this exception was raised, it means another bean for same name is already registered,
// revert to it
bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
synchronized (beanFactory.getSingletonMutex()) {
if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
return (T) beanFactory.getSingleton(domainableBeanNameWithConf);
} else {
T bean = (T) beanFactory.
createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
beanFactory.registerSingleton(domainableBeanNameWithConf, bean);
return bean;
}
}
}

return bean;
return null;
}

public static Class<?> purge(final String implementation) {
Expand Down