SpringBoot CollectionBinder cause original collection object be empty in concurrent case.

We recently found list type object in RefreshScope configuration adventitiously be empty when use config center to refresh config data. And then we find CollectionBinder.merge method will get the original object and set to existingCollection, then clear it and addAll new config data. When a thread access this collection config object will lead to get a empty collection after the clear method invoke and before the addAll method invoke. It seems a concurrent bug here.

https://github.com/spring-projects/spring-boot/blob/47516b50c39bd6ea924a1f6720ce6d4a71088651/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java#L57-L66

To avoid this case in the current/past versions, we can wrap the collection in config object with a new collection, and then existingCollection.clear method will clear the new collection, so the original collection is still with all elements util refresh phase done to set new collection here.

@RefreshScope
@ConfigurationProperties(prefix = "xx")
public class Config2 {
    private List<String> demos = new ArrayList<>();
    public void setDemos(List<String> demos) {
        this.demos = demos;
    }
    public List<String> getDemos() {
        List<String> list = new ArrayList<>(this.demos.size());
        list.addAll(this.demos);
        return list;
    }
}

Advance suggest to fix this issue is to new a collction to wrap existingCollection at first in merge method, so clear method will not affect the original collection instance in configuration object. It will be safe in concurrent case.

Comment From: kimmking

It is similar about https://github.com/spring-projects/spring-boot/issues/32877

Comment From: wilkinsona

Thanks for the report. The Binder in Spring Boot does not expect properties to be read on one thread while another thread is binding those properties. It is happening in your case due to the way that refresh has been implemented in Spring Cloud. Beyond collection binding, I believe you could also see problems where a reading thread sees a mixture of old and new properties while binding is in progress. This will have to be addressed in Spring Cloud to ensure that configuration properties instances that are being refreshed do not become visible until property binding has completed. This could perhaps be achieved via proxying with the instance that backs the proxy being swapped once binding has completed.

Comment From: rechardguo

@kimmking No, 32877 is not similar. I understand your case config empty is because refresh thread do clear and at this moment another thread get config so it get empty, So you write a copy and return to avoid merge thread clear the orignial one. 32877 is MapBinder handle for map,Code as below SpringBoot CollectionBinder cause original collection object be empty in concurrent case. It does not cause empty issue, It will cause some key we delete in config server still keep in config object.

@wilkinsona I am wondering why CollectionBinder#merge not write as

 return copyIfPossible(additional);

instead of

 existingCollection.clear(); 
 existingCollection.addAll(additional); 
 return copyIfPossible(existingCollection);

Comment From: philwebb

@rechardguo If I remember correctly, we mutate the collection because we might not have a setter to call. https://github.com/spring-projects/spring-boot/issues/12322 has more background about why we also call copyIfPossible.