open-feature
[FEATURE] Init and Shutdown functions should take `context.Context` parameter
Requirements
The StateHandler interface defines two functions: Init and Shutdown. https://github.com/open-feature/go-sdk/blob/dd4cf71f8226026a71d51d1edcfd1026c965be15/openfeature/provider.go#L64-L67
There is also a top-level Shutdown function: https://github.com/open-feature/go-sdk/blob/dd4cf71f8226026a71d51d1edcfd1026c965be15/openfeature/openfeature.go#L90-L92
Section 2.4 of the specification says the following (emphasis mine).
Many feature flag frameworks or SDKs require some initialization before they can be used. They might require the completion of an HTTP request, establishing persistent connections, or starting timers or worker threads. The initialization function is an ideal place for such logic.
The types of initialization mentioned above normally require a context.Context parameter to be passed. For example, to make an HTTP request, one most definitely should be passing a context.Context parameter.
As for Shutdown, a context can be used to manage timeouts or cancellation.
The context is also the mechanism through which OpenTelemetry spans and other request-scoped values are propagated. If there is any chance at all that a provider author might perform I/O in the Init and Shutdown functions, then these functions need to have a context parameter.
The context package documentation has more details on what contexts are used for and where they should be used.
I propose that the Init and Shutdown functions linked at the top take a context.Context parameter for the reasons outlined above. While this is a breaking change, I think it is acceptable given that both section 2.4 and section 2.5 of the specification are currently experimental.
This is a good idea, I think.
We can change the provider interface in a breaking way without violating our contract because, as @sahidvelji says, it's not a breaking change... it wouldn't even impact application authors, really only provider authors.
However, I think changing the API-level .Shutdown function would be a severe breaking change, technically. It would impact application authors and application integrators.
Is there a clean way to do this without breaking the existing Shutdown contract?
The other option is to reconsider a 2.0, which we could also use as an opportunity to fix some other issues and remove some deprecations.
However, I think changing the API-level .Shutdown function would be a severe breaking change, technically. It would impact application authors and application integrators.
While that is true, it would be very easy to fix. Both application authors and application integrators would simply do
- openfeature.Shutdown()
+ openfeature.Shutdown(ctx) // or context.Background()
Is there a clean way to do this without breaking the existing Shutdown contract?
If the above is not preferred, then the alternative would be to create a new top-level function ShutdownWithContext(ctx context.Context) and have the Shutdown() func use context.Background internally.
The other option is to reconsider a 2.0, which we could also use as an opportunity to fix some other issues and remove some deprecations.
Yes, I think a 2.0 is needed to include generics support as per #158 regardless. But either of the two options above should suffice for now.
Somewhat related: the Shutdown function does not return an error value, but there could be cases where a shutdown is not successful, in which case the error should be propagated back to the caller.
I think a 2.0 is a good idea, generally.
I just realized though that shutdown is still experimental, so I'm in favor of just doing the breaking changes you describe (return error and take context) and collecting changes for a 2.0 separately.
Due to where Shutdown and Init are used in the code, SetNamedProvider, SetNamedProviderAndWait, SetProvider, and SetProviderAndWait would all also need to take a context parameter to make this work.
hmmm.... that's definitely more of a breaking change.
What do you think @sahidvelji ? Should we just "bite the bullet" and release a 2.0? I'm interested in your perspective.
One of the reasons we haven't released a 2.0 already was due to the difficulty of releasing/maintaining v2+ versions in the past, back when many people were still using GOPATH style development and when it was standard to create a whole new /v2 directory and copy all the source there. I don't think we have that constraint anymore and can now do a v2 by simply changing the package name in the go.mod here's a v72 for example.
We are already in 2.0 in other SDKs as well, so I'm not against it!
cc @thomaspoignant @kinyoklion @lukas-reining @beeme1mr
I am not against moving to 2.0, but as you said we will have to keep supporting the 1.0 in the repo since GO is working like this for the go modules.
If we don't want to make breaking changes, we can also create extra functions
func InitWithContext(ctx context.Context, evaluationContext EvaluationContext) error
func ShutdownWithContext(ctx context.Context)
This is a pattern I have seen in some go library when you want to offer the 2 ways to access a function. The main advantage I see doing that is avoiding breaking changes and still give the opportunity to pass a context.
A random search in the k8s repo, shows that this is pretty common to have this kind of patterns
https://github.com/search?q=repo%3Akubernetes%2Fclient-go%20WithContext&type=code
Yes, I do see a need for a v2 of this SDK, primarily to support generics and fix some anti-patterns throughout the codebase to align with language best practices.
If we don't want to make breaking changes, we can also create extra functions
func InitWithContext(ctx context.Context, evaluationContext EvaluationContext) error func ShutdownWithContext(ctx context.Context) This is a pattern I have seen in some go library when you want to offer the 2 ways to access a function. The main advantage I see doing that is avoiding breaking changes and still give the opportunity to pass a context.
That is true, but would also require us to create SetNamedProviderWithContext, SetNamedProviderAndWaitWithContext, SetProviderWithContext, and SetProviderAndWaitWithContext, which just becomes a mess. I also don't have an immediate need for context aware Init and Shutdown personally, so I can wait until we have a v2.
I am not against moving to 2.0, but as you said we will have to keep supporting the 1.0 in the repo since GO is working like this for the go modules.
While we do need to keep the v1 code around, we do not need to support v1 indefinitely. Once v2 is released, we would deprecate v1 and support it for some specified period of time, after which all future changes occur in v2.
I would like to discuss some of the specifics around v2 of this SDK in the next community meeting, before we go ahead with it.
While we do need to keep the v1 code around, we do not need to support v1 indefinitely. Once v2 is released, we would deprecate v1 and support it for some specified period of time, after which all future changes occur in v2.
This is actually easily done, we've done it before with other SDKs. It basically involves keeping a v1 branch (without the go.mod update) and configuring release please to release from that as well. Then we can merge hotfixes to that branch to support 1.x for some amount of time.
I would like to discuss some of the specifics around v2 of this SDK in the next community meeting, before we go ahead with it.
Sounds great!