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

Mismatch in ConfigProviderResolver instances after boot #4512

Closed
jmartisk opened this issue Oct 11, 2019 · 15 comments
Closed

Mismatch in ConfigProviderResolver instances after boot #4512

jmartisk opened this issue Oct 11, 2019 · 15 comments
Assignees
Labels
area/config kind/bug Something isn't working
Milestone

Comments

@jmartisk
Copy link
Contributor

During runtime init we explicitly build an instance of SimpleConfigurationProviderResolver and pass it to ConfigProviderResolver.setInstance()[1][2], but there is also the static field ConfigProvider.INSTANCE [3] that initializes by calling ConfigProviderResolver.instance() BEFORE we do ConfigProviderResolver.setInstance(), which leads to instantiation of a SmallRyeConfigProviderResolver (!!!). As a consequence, the fields ConfigProviderResolver.instance and ConfigProvider.INSTANCE, which should contain the same instance of SimpleConfigurationProviderResolver, actually contain not only two different instances, but they are of two different classes.

Normally these two resolvers resolve to equivalent config instances, but this issue manifests when an application registers a custom Config using ConfigProviderResolver.instance().registerConfig(), becase after this, two different Config instances will be returned from ConfigProviderResolver.instance().getConfig() (this will contain the new dynamically created one) and ConfigProvider.getConfig() (this will contain the original one created during boot).

I know we don't quite support registering a custom Config at runtime, but the Metrics TCK is doing this in one test, we should at least get this test working.

IIUC we don't want to use SmallRyeConfigProviderResolver in Quarkus at all (and instead use the SimpleConfigurationProviderResolver) so we need to get rid of instantiating the former.

[1] https://github.com/quarkusio/quarkus/blob/0.24.0/core/deployment/src/main/java/io/quarkus/deployment/steps/ConfigurationSetup.java#L515
[2] https://github.com/eclipse/microprofile-config/blob/1.3/api/src/main/java/org/eclipse/microprofile/config/ConfigProvider.java#L74
[3] https://github.com/eclipse/microprofile-config/blob/1.3/api/src/main/java/org/eclipse/microprofile/config/spi/ConfigProviderResolver.java#L44

@jmartisk jmartisk added kind/bug Something isn't working area/config labels Oct 11, 2019
@jmartisk
Copy link
Contributor Author

Hey @dmlloyd, is it really required by design that we use SimpleConfigurationProviderResolver instead of SmallRyeConfigProviderResolver? Because another problem that blocks the mentioned test is:
While SimpleConfigurationProviderResolver's very simple implementation allows calling registerConfig and then retrieving the new config, if the app later calls releaseConfig (https://github.com/eclipse/microprofile-metrics/blob/2.0.2/tck/api/src/main/java/org/eclipse/microprofile/metrics/tck/GlobalTagsTest.java#L117) then the config gets set to null and the application will no longer be able to retrieve any config, which breaks subsequent tests.
To pass the TCK we need an implementation "smart enough" that after calling registerConfig and releaseConfig, the resolver will return the original config.

How about we, for example, enhance the SimpleConfigurationProviderResolver at least to the extent that it will keep a reference to the first Config that gets registered (this will be the global config from boot), and then will revert to it after a releaseConfig call?

@jmartisk
Copy link
Contributor Author

CC @kenfinnigan this is the issue with the MP Metrics TCK I mentioned on yesterday's call

@jmartisk jmartisk self-assigned this Oct 11, 2019
@dmlloyd
Copy link
Member

dmlloyd commented Oct 11, 2019

This is a known issue. There is a fix pending in SmallRye Config which will allow us to customize the config provider resolver to the degree that we need to do so.

@kenfinnigan
Copy link
Member

@dmlloyd is there a SmallRye Config issue already that this relates to?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 11, 2019

It was part of smallrye/smallrye-config#168.

@kenfinnigan
Copy link
Member

So we just need a release of it, and update Quarkus?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 11, 2019

Basically it's pending release and corresponding Quarkus fixes, but I have been waiting to ask for a release until all the Quarkus work is done.

@kenfinnigan
Copy link
Member

I know Sonatype is a pain at the moment, but do you want me to try releasing Config? Or there's more work still?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 11, 2019

It won't fix the problem until the Quarkus config setup is modified. We could do a separate pull for that but I'm already in the midst of another mega config fix which will cover that so I was just going to do it all at once, as is my wont.

@kenfinnigan
Copy link
Member

Ok, but are there more changes to SRye Config needed? Or you think we're done and a release would help you finalize the mega fix?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 11, 2019

There might or might not be more changes needed: I won't be sure until I finish the Quarkus change, TBH.

@kenfinnigan
Copy link
Member

Fair enough, keep us posted

@dmlloyd dmlloyd self-assigned this Oct 24, 2019
@kenfinnigan
Copy link
Member

How's the big config PR going @dmlloyd?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 30, 2019

Working, sort of. Just fixing some glitches. Then I have to add a bunch more tests.

@kenfinnigan
Copy link
Member

Ok, if there's anything I can do to help, just let me know

dmlloyd added a commit to dmlloyd/quarkus that referenced this issue Nov 11, 2019

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
@gsmet gsmet added this to the 1.1.0 milestone Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants