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

[bug] Multiple providers bound to one "name", and associated issues #192

Closed
Tracked by #126
benjiro opened this issue May 31, 2023 · 11 comments
Closed
Tracked by #126

[bug] Multiple providers bound to one "name", and associated issues #192

benjiro opened this issue May 31, 2023 · 11 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@benjiro
Copy link
Member

benjiro commented May 31, 2023

Spec: a4ffec3
See: open-feature/dotnet-sdk#126

The spec allows a single instance of a provider to be bound to multiple names (even if it didn't allow this, any naive implementation would probably allow it to happen). We either need to prevent it or specify how it should work. We may want to specify that providers that are still bound to any client name are not shut down. - @toddbaert

I consider this a spec bug personally, because it constitutes a problematic ambiguity in the spec. - @toddbaert

cc @justinabrahms

Quote from @kinyoklion 👍

Basic case.

var provider1 = new TestProvider();

openFeature.SetProvider("a",  provider1);

// Call init on provider1?

openFeature.SetProvider("a",  new TestProvider());

// Call shutdown on provider 1?

// Call init on new provider.

Two Names Scenario 1.

var provider = new TestProvider();

openFeature.SetProvider("a", provider);

// Call init on provider?

openFeature.SetProvider("b", provider);

// Provider already has init called. Do we call it again?

Two Names Scenario 2.

var provider1 = new TestProvider();

openFeature.SetProvider("a", provider1);
openFeature.SetProvider("b", provider1);

openFeature.SetProvider("b",  new TestProvider());

// We don't want to shutdown provider1, because it is still registered to "b".

@benjiro benjiro added the enhancement New feature or request label May 31, 2023
@benjiro benjiro self-assigned this May 31, 2023
@benjiro
Copy link
Member Author

benjiro commented May 31, 2023

Let's continue the discussion about the following scenario described by @kinyoklion here open-feature/dotnet-sdk#129 (comment)

@toddbaert toddbaert transferred this issue from open-feature/dotnet-sdk May 31, 2023
@toddbaert toddbaert changed the title Implement initialization/shutdown support open-feature/spec@a4ffec3 Multiple providers bound to one "name", and associated issues May 31, 2023
@toddbaert toddbaert added the bug Something isn't working label May 31, 2023
@toddbaert toddbaert changed the title Multiple providers bound to one "name", and associated issues [bug] Multiple providers bound to one "name", and associated issues May 31, 2023
@toddbaert
Copy link
Member

cc @lopitz @lukas-reining

@toddbaert
Copy link
Member

@benjiro FYI I moved this from the dotnet-sdk repo.

@lukas-reining
Copy link
Member

I totally agree.
Also found that and implemented it in the JS SDK the way that this does not happen for the case between named and unnamed providers.
But scenario 2 is not handled by the JS SDK now, I will fix that @toddbaert.

@toddbaert
Copy link
Member

toddbaert commented May 31, 2023

I totally agree. Also found that and implemented it in the JS SDK the way that this does not happen for the case between named and unnamed providers. But scenario 2 is not handled by the JS SDK now, I will fix that @toddbaert.

Let's wait for now to build some consensus, just so that you dont implement something that you have to change. Do you agree with the solution I've vaguely proposed?

Any alternatives?

@lukas-reining
Copy link
Member

Yes I totally agree with that @toddbaert, this is fixing at least the shutdown for me.
I already did that, was just 3 lines of code and a test.
Will add the PR and we can merge it or change it once we have concencus what do you think?

@toddbaert
Copy link
Member

Yes I totally agree with that @toddbaert, this is fixing at least the shutdown for me. I already did that, was just 3 lines of code and a test. Will add the PR and we can merge it or change it once we have concencus what do you think?

For now maybe add the PR here for reference.

@lukas-reining
Copy link
Member

I fixed that in the JS SDK now. I had this check for the default provider already, but missed to do it for several named clients.
open-feature/js-sdk#444

@toddbaert
Copy link
Member

Reading the spec a bit more, I think 1.1.2.2 and 1.1.2.3 technically addresses these concerns, though the non-normative helper-text could be improved.

@justinabrahms
Copy link
Member

Thanks Todd.
I agree with what's been stated:

basic:

  • yes call init there
  • yes, call shutdownthere

2 names:

  • call init when set
  • if already init'd, don't do it again

2 names 2:

  • don't shut down p1

@toddbaert
Copy link
Member

I think this is taken care of with #193. Please re-open if you disagree, @benjiro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants