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

[benjiro]

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]

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

[toddbaert]

cc @lopitz @lukas-reining

[toddbaert]

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

[lukas-reining]

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]

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]

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]

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]

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]

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]

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]

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