Skip to content

fix: old provider not shutting down when the new provider fails to initialize#98

Merged
NeaguGeorgiana23 merged 5 commits into
mainfrom
shut_down_old_provider
May 27, 2026
Merged

fix: old provider not shutting down when the new provider fails to initialize#98
NeaguGeorgiana23 merged 5 commits into
mainfrom
shut_down_old_provider

Conversation

@NeaguGeorgiana23
Copy link
Copy Markdown
Contributor

@NeaguGeorgiana23 NeaguGeorgiana23 commented May 12, 2026

This PR

Ensures that a displaced feature provider is always shut down when a new provider is set, preventing resource leaks when the new provider fails to initialize.
Previously, the old provider was removed from the internal repository map immediately upon setting a new one, but Shutdown() was only called if the new provider successfully initialized. If the new provider's Init() failed, the old provider remained active but unreferenced, leaking its resources.

Related Issues

Fixes #69

…tialize.

Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
@NeaguGeorgiana23 NeaguGeorgiana23 requested review from a team as code owners May 12, 2026 09:23
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the ProviderRepository to unconditionally shut down the old provider during initialization, regardless of whether the new provider successfully reaches a ready state. Correspondingly, the test suite has been updated to verify that the old provider is indeed shut down even when the new provider's initialization fails. I have no feedback to provide.

Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Copy link
Copy Markdown
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a dedicated test for this but I won't block it. Feel free to merge, or add a test then merge.

Sorry, there is one. I just missed it in some rename noise!

@NeaguGeorgiana23 NeaguGeorgiana23 merged commit aa27439 into main May 27, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specification Compliance Review

3 participants