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

Is the return value of Config#getConfigSources() specified to be immutable and unchanging? #371

Closed
ljnelson opened this issue Jun 8, 2018 · 16 comments
Assignees
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Milestone

Comments

@ljnelson
Copy link
Contributor

ljnelson commented Jun 8, 2018

In #370 @struberg notes that "Config infra is basically immutable". Is there a note somewhere in the specification that indicates that, for example, Config#getConfigSources() must return an unchanging Set of ConfigSources when invoked? Should there be?

Similarly, what is the contract for ConfigSourceProvider#getConfigSources(ClassLoader)? Is it required or prohibited or undefined for a ConfigSourceProvider implementation to return a dynamic Set?

@struberg
Copy link
Contributor

The specification describes all the setup and discovery which happens at startup. And that's it.
So from that you can assume that those parts are not intended to get altered later.

@ljnelson
Copy link
Contributor Author

Well, the specification doesn't actually say anything about how ConfigSources and Config are linked together. As you say, I assume that once ConfigSources are discovered by the service provider mechanism, then they're supposed to be used by Config and should? must? be returned by Config#getConfigSources() but I didn't see anything that says they must be.

Also, the specification has a section on dynamic ConfigSource providers. I don't think I can make any assumptions about what they can do. Or are you saying, yes, I can, they must be called once and must produce immutable unchanging Sets themselves?

@struberg struberg self-assigned this Jun 11, 2018
@struberg
Copy link
Contributor

Indeed that might be good to clarify.

@Emily-Jiang
Copy link
Member

@struberg and I talked this a bit more in today's hangout. We think it is best left unspecified so that implementations can have freedom to decide whether they want to support dynamically reloaded config sources.

@ljnelson
Copy link
Contributor Author

Thanks; in that case, I suppose that it must be specified that the return value must be safe for concurrent use by multiple threads? What must I synchronize on when iterating over it? Itself?

@Emily-Jiang
Copy link
Member

@ljnelson it is best for you to join our MP Config weekly hangout to discuss your use case. Can you join next week? Please let me know on gitter if you need help with the meeting calendar.

@ljnelson
Copy link
Contributor Author

I'll try to make it.

(For background, this isn't really connected with a use case: this is me simply trying to understand how I should iterate through the return value returned by getConfigSources(). Either it is safe for concurrent use by multiple threads or it is not; you can't leave this unspecified.)

@ljnelson
Copy link
Contributor Author

ljnelson commented Jul 18, 2019

Related: #369, #370, #395, #431, #457

@jmesnil jmesnil added clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API mpconfig-1.4 labels Aug 2, 2019
@dmlloyd
Copy link
Contributor

dmlloyd commented Sep 12, 2019

I'll note that it's always possible to provide a ConfigSource that itself delegates to a dynamically changing list of ConfigSources. So, requiring that the set of config sources be immutable with the idea that the configuration would be immutable as a result as a matter of principle is a bit of a shaky premise, unless such a requirement is also imposed on the ConfigSource itself.

That said, it should definitely be specified that the config sources iterable returned from getConfigSources() can be iterated in a thread-safe manner, including allowing concurrent iteration. This relates also to #369 for this reason: the specifications should (IMO) be similar.

@Emily-Jiang
Copy link
Member

As today's hangout, the spec will not enforce whether the list is immutable or not. However, we would like to clarify the method getConfigSources() can be iterated in a thread-safe manner as per #371 (comment)

@Emily-Jiang Emily-Jiang assigned carlosdlr and unassigned struberg Nov 22, 2019
@Emily-Jiang Emily-Jiang self-assigned this Jan 21, 2020
@Emily-Jiang
Copy link
Member

@carlosdlr I'll take this one and add one line on the javadoc so that it can be in the release of Config 1.4.

@carlosdlr
Copy link

Thank you @Emily-Jiang

Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue Jan 21, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue Jan 24, 2020
…ckstyle error introduced be a previous PR

Signed-off-by: Emily Jiang <[email protected]>
Emily-Jiang added a commit that referenced this issue Jan 24, 2020
#371 improve javadoc on getConfigSources(), also fixed the checkstyle…
@kenfinnigan
Copy link
Contributor

Why was this issue closed?

There was a lengthy discussion on the merged PR with @ljnelson and @radcortez that I believe impacts the closure of this issue

@radcortez
Copy link
Contributor

Yes, I believe #510 should be clarified before closing #371.

@ljnelson
Copy link
Contributor Author

I would like to kindly request that this issue be reopened as (a) it was not resolved and (b) no reason was given for its closing.

@emattheis
Copy link
Contributor

At the risk of derailing this thread, can someone point me to any background on why config sources are exposed by Config in the first place? The inherently stateful nature of a ConfigSource makes this part of the spec problematic for specifiers and implementors alike. For instance, the fact that someone could obtain the source and close it. Do we need to consider proxying sources to avoid that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Projects
None yet
Development

No branches or pull requests

9 participants