Skip to content
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.

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

ConfigurationProperties changes can not be seen when EnvironmentChangedEvent happens #587

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aftersss
Copy link

@aftersss aftersss commented Aug 5, 2019

We use the following code and expect that after refresh, someProperties.getProperties() will be changed, but it does not. only when refresh is triggered one more time, the change can be seen.

@Configuration
@EnableConfigurationProperties(SomeAutoConfiguration.SomeProperties.class)
public class SomeAutoConfiguration {
    
    @Resource
    SomeProperties someProperties;
    
    @EventListener(EnvironmentChangeEvent.class)
    public void onEnvChanged(){
        //after refresh, someProperties.getProperties() is not changed!
        System.out.println(someProperties.getProperties());
    }

    @ConfigurationProperties("some")
    public static class SomeProperties {

        private Map<String, String> properties = new LinkedHashMap<>();

        public Map<String, String> getProperties() {
            return properties;
        }

        public void setProperties(Map<String, String> properties) {
            this.properties = properties;
        }

    }
}

This PR improves this, give ConfigurationPropertiesRebinder a very high order so users can always see ConfigurationProperties changes when EnvironmentChangedEvent happens.

…lways see `ConfigurationProperties` changes when `EnvironmentChangedEvent` happens
@ryanjbaxter
Copy link
Contributor

Shouldn't the @ConfigurationProperties class above have an @RefreshScope annotation on it to make this work?

@aftersss
Copy link
Author

aftersss commented Aug 6, 2019

Shouldn't the @ConfigurationProperties class above have an @RefreshScope annotation on it to make this work?

Maybe the creation of a bean is very expensive(for example, DataSource), and the properties of this bean don't change due to every refresh(most times refresh is to change other properties, not properties of this bean). If we use @RefreshScope, the bean will be recreated due to every refresh, but we only want to change some properties of this bean, without recreating, considering that recreating is too expensive.

@spencergibb
Copy link
Member

I still don't see what this fixes. Can you provide a test that fails prior to this change?

@aftersss
Copy link
Author

aftersss commented Aug 6, 2019

Here is a more meaningful example:

import java.util.LinkedHashMap;
import java.util.Map;
import javax.annotation.Resource;
import javax.sql.DataSource;

import org.apache.commons.dbcp2.BasicDataSource;

import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.cloud.context.environment.EnvironmentChangeEvent;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.event.EventListener;

@Configuration
@EnableConfigurationProperties(MyDataSourceConfiguration.DataSourceDynamicProperties.class)
public class MyDataSourceConfiguration {
    
    @Resource
    DataSourceDynamicProperties dataSourceDynamicProperties;
    @Resource
    ApplicationContext applicationContext;

    @Bean("myDataSource")
    public DataSource myDataSource(){
        BasicDataSource dataSource = new BasicDataSource();
        dataSource.setUrl("***");
        dataSource.setUsername("***");
        dataSource.setPassword("***");
        dataSource.setMaxTotal(dataSourceDynamicProperties.getMaxTotal());
        return dataSource;
    }
    
    @EventListener(EnvironmentChangeEvent.class)
    public void onEnvChanged(){
        BasicDataSource dataSource = (BasicDataSource)applicationContext.getBean("myDataSource", DataSource.class);
        dataSource.setMaxTotal(dataSourceDynamicProperties.getMaxTotal());
        //dataSource.getMaxTotal() is still 10 after refresh!
    }

    @ConfigurationProperties("my.datasource")
    public static class DataSourceDynamicProperties {

        private int maxTotal = 10;

        public int getMaxTotal() {
            return maxTotal;
        }

        public void setMaxTotal(int maxTotal) {
            this.maxTotal = maxTotal;
        }
    }
}

What I want to do is refresh the datasource's MaxTotal property dynamically using the following steps:

  1. When this application is first started, maxTotal is 10.(default value)

  2. put key-value property pair my.datasource.maxTotal=20 into configServer.

  3. curl -XPOST http://localhost/refresh to refresh my application.

  4. my expect is dataSource.getMaxTotal() being changed to 20, but this is still 10.

I found that this is because that ConfigurationPropertiesRebinder listens to EnvironmentChangeEvent too, and the order of MyDataSourceConfiguration.onEnvChanged and ConfigurationPropertiesRebinder is undefined, so if MyDataSourceConfiguration.onEnvChanged is executed first, it can not see the change in DataSourceDynamicProperties. So I think ConfigurationPropertiesRebinder can be assigned with a very high order to let it be executed first, then MyDataSourceConfiguration.onEnvChanged can always see the change.

@ryanjbaxter
Copy link
Contributor

I still dont see why adding @RefreshScope to DataSourceDynamicProperties wouldnt do what you need

@aftersss
Copy link
Author

aftersss commented Aug 7, 2019

Adding @RefreshScope to DataSourceDynamicProperties still don't work, to make this work, another thing should be done too: change @EventListener(EnvironmentChangeEvent.class) to @EventListener(RefreshScopeRefreshedEvent.class) . But If I do this, @RefreshScope is useless, it can be removed, and this still works, so this is confusing.

I think supporting EnvironmentChangedEvent in this example is better, in my example, if I change the code to add order like the following, it still works:

 @EventListener(EnvironmentChangeEvent.class)
@Order(Ordered.LOWEST_PRECEDENCE) //add order
    public void onEnvChanged(){
        BasicDataSource dataSource = (BasicDataSource)applicationContext.getBean("myDataSource", DataSource.class);
        dataSource.setMaxTotal(dataSourceDynamicProperties.getMaxTotal());
        .......
    }

This PR is to let users does not need to care about the order. just like what OrderedCharactorEncoding does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants