Skip to content

ConfigurationPropertiesRebinder exposes invalid state racily#1709

Open
obourgain wants to merge 1 commit into
spring-cloud:mainfrom
obourgain:racy_ConfigurationPropertiesRebinder
Open

ConfigurationPropertiesRebinder exposes invalid state racily#1709
obourgain wants to merge 1 commit into
spring-cloud:mainfrom
obourgain:racy_ConfigurationPropertiesRebinder

Conversation

@obourgain

Copy link
Copy Markdown

ConfigurationPropertiesRebinder updates the state of the bean annotated @ConfigurationProperties without any synchronization and exposes invalid state while doing the reset.

Before #1680, ConfigurationPropertiesRebinder called the Binder to replace the values of the bean. This was already racy as another thread may observe state that is a mix of before/after refresh of the properties, and in the case of more complex types like a Map, may access it concurrently while it is mutated without any synchronization. Similar issue can arise from the destroy/init cycle of the bean while it may be in use from the app.

Since #1680, the problem is even worse. A new instance of the same class as the bean is created, so as to get the default field values and those set in the constructor. The state of this new object, which is mostly irrelevant for an application, is copied over the bean, here again without any synchronization. Application threads can yet again observe a state equivalent of the new instance, or a Frankenstein monster of before the properties refresh, after the properties refresh and brand new instance fields.

This is not a theoretical issue, this actually bits us in production. A config class annotated @ConditionalOnProperty that is activated when the context starts can expect the given property to exist and not spuriously become null when reading it later at runtime.

I added a test that show the race.
The test starts a simple timer to wait for 1 second, then starts another that calls rebind in a loop, and the teest thread just reads a property. There is no update of the property, yet in a single second we can observe 10s of millions of invalid state. e.g.

expected: 0
 but was: 675703745
Expected :0
Actual   :675703745

When disabling the bean reset to default values, we can observe no invalid state.

AFAIK, you can not solve the rebinding issue with this kind of code and without synchronization with the use-site. You have two choices: 1- use a proxy, but then it gets some overhead, and you can't support final classes/records. It may has well be another can of worms. 2- do not try to make the property rebind invisible to application code but embrace it. We have a working internal implementation of such a thing, it basically uses something akin to Supplier<MyProperties> so application code can do supplier.get() and get a consistent snapshot of properties. When a properties refresh happens, we just return a fresh newly bound instance on the next call to .get().

ConfigurationPropertiesRebinder updates the state of the bean annotated @ConfigurationProperties without any synchronization and exposes invalid state while doing the reset.

Before spring-cloud#1680, ConfigurationPropertiesRebinder called the Binder to replace the values of the bean. This was already racy as another thread may observe state that is a mix of before/after refresh of the properties, and in the case of more complex types like a Map, may access it concurrently while it is mutated without any synchronization.
Similar issue can arise from the destroy/init cycle of the bean while it may be in use from the app.

Since spring-cloud#1680, the problem is even worse.
A new instance of the same class as the bean is created, so as to get the default field values and those set in the constructor.
The state of this new object, which is mostly irrelevant for an application, is copied over the bean, here again without any synchronization.
Application threads can yet again observe a state equivalent of the new instance, or a Frankenstein monster of before the properties refresh, after the properties refresh and brand new instance fields.

This is not a theoretical issue, this actually bits us in production. A config class annotated `@ConditionalOnProperty` that is activated when the context starts can expect the given property to exist and not spuriously become null when reading it later at runtime.

I added a test that show the race.
The test starts a simple timer to wait for 1 second, then starts another that calls rebind in a loop, and the teest thread just reads a property. There is no update of the property, yet in a single second we can observe 10s of millions of invalid state.
e.g.
```
expected: 0
 but was: 675703745
Expected :0
Actual   :675703745
```

When disabling the bean reset to default values, we can observe no invalid state.

AFAIK, you can not solve the rebinding issue with this kind of code and without synchronization with the use-site. You have two choices:
1- use a proxy, but then it gets some overhead, and you can't support final classes/records. It may has well be another can of worms.
2- do not try to make the property rebind invisible to application code but embrace it.
We have a working internal implementation of such a thing, it basically uses something akin to `Supplier<MyProperties>` so application code can do `supplier.get()` and get a consistent snapshot of properties. When a properties refresh happens, we
just return a fresh newly bound instance on the next call to `.get()`.
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.

2 participants