ConfigurationPropertiesRebinder exposes invalid state racily#1709
Open
obourgain wants to merge 1 commit into
Open
ConfigurationPropertiesRebinder exposes invalid state racily#1709obourgain wants to merge 1 commit into
obourgain wants to merge 1 commit into
Conversation
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()`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
@ConditionalOnPropertythat 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.
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 dosupplier.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().