Open
Conversation
…ay), to reduce test flakiness
`ConfigServerClientOptions` are now re-evaluated from initial options + configuration on every settings change, instead of being mutated in-place. This prevents stale or torn reads when the provider runs concurrently (e.g. polling timer vs reload). Key changes: - The provider clones options on each configuration reload, starting from the initial options passed via code, then applying configuration on top. This ensures code-level defaults are restored when keys are removed from configuration. - Client settings (spring:cloud:config:*) are no longer written into the provider's data dictionary, eliminating a circular feedback loop where the provider's own output could influence its input. - Discovery lookup results are tracked separately and applied on top of the options snapshot at load time, rather than mutating shared state. - Client certificate configuration is moved from the source's `Build` method into `ConfigureConfigServerClientOptions`, so it participates in the options pipeline. - `ConfigServerClientOptions.Clone()` is introduced to produce isolated snapshots, preventing tearing when options are read during a concurrent reload. A bug was fixed that prevented using global certificates. - An internal `HttpClientHandler` parameter is threaded through the source and builder extensions, enabling direct handler injection for tests without reflection. - `IOptionsChangeTokenSource<ConfigServerClientOptions>` is registered in DI so that `IOptionsMonitor` properly triggers on configuration changes.
The provider had several concurrency and lifecycle issues: - Vault token renewal timers were created unboundedly on every HTTP request that carried a token, leaking timers that were never disposed. Vault renewal is now managed as a single timer with the same lifecycle as the polling timer. - Timer management, handler configuration, and disposal could race with each other without synchronization. A lifecycle lock now guards these operations, while non-blocking try-enter locks prevent timer callbacks from queueing up. - The HTTP client handler's certificates were reconfigured on every HTTP request, racing with concurrent requests sharing the same handler. Certificate and validation configuration is now applied once per settings change under the lifecycle lock, with certificates cleared before re-adding to prevent unbounded accumulation across reloads. - Options cloning did not copy the certificate issuer chain, losing intermediate CA certificates across reloads. - Disposal is now coordinated via a volatile flag so that in-flight timer callbacks and Load() calls exit gracefully instead of throwing unobserved exceptions.
Polled configuration reloads now refresh the discovery service lookup before fetching from Config Server, enabling detection of server address changes between polling intervals. Unlike initial load, polled reload does not apply FailFast or retry semantics. Fixed a race where concurrent calls to LoadInternalAsync could create multiple ConfigServerDiscoveryService instances, each constructing their own temporary DI container and discovery clients.
Each configuration reload re-registered a new change callback via RegisterChangeCallback, without disposing the previous one. Rapid reloads could accumulate stale callbacks, causing redundant OnSettingsChanged invocations on multiple threads. Replaced with a single ChangeToken.OnChange registration in the constructor, which automatically handles re-registration and is disposed on shutdown.
Replace shared mutable HttpClientHandler with a per-request factory, eliminating the race where OnSettingsChanged reconfigured a handler concurrently in use by HTTP requests. Production code creates and disposes a fresh handler per request; tests inject a factory for mocking. Replace _isDisposed flag with CancellationToken-based shutdown, enabling in-flight HTTP requests in timer callbacks to terminate promptly on disposal instead of running to completion.
Contributor
Summary - All Code Coverage (ubuntu-latest)
|
- Improved log messages (and don't log multiple times), fix exception stack traces - Don't remote-fetch twice at startup (load + timer that immediately fired) - Moved options bind and remote-fetch from constructor to Load, fix combination with placeholder
|
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
1 participant
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.
Description
Sandboxusage withMemoryFileProvider(without sleeps)IOptionsMonitor<ConfigServerClientSettings>to return the same settings as the provider uses internallyHttpClientHandlerfor each request because reusing with changing certificates is not thread-safeQuality checklist
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.