open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
666 stars 34 forks source link

[Question] `API.shutdown()` required in Go? #189

Closed toddbaert closed 1 year ago

toddbaert commented 1 year ago

1.6.1 requires an API.shutdown() function. After a few discussions with contributors I'm not 100% sure this is necessary, particularly for Go.

The purpose of the API.shutdown() function is for providing a means to initiate a graceful shutdown. For example, in Java, you'd catch a SIGINT in your Java app, and then that handler would call API.Shutdown() to cleanup all the polling, DB connections, workers, and flush telemetry or whatever other cleanup providers need done.

However in Go, the Context provides some of this functionality. A better pattern for Go might be to create a provider with a NewMyProvider function (which the provider author defines) which takes Context. That provider listens for ctx.done() and handles their own shutdown. Application authors simply emit the cancellation event and everything is cleaned up.

We may want to relax 1.6.1m making it a "MAY" instead of a "MUST", and add some non-normative text like:

Implementations can choose to leverage language idioms such as auto-disposable interfaces and other means of cancellation signal propagation to allow for graceful shutdown.

@open-feature/sdk-golang-approvers @open-feature/sdk-golang-maintainers

weyert commented 1 year ago

I think that's reasonable

thomaspoignant commented 1 year ago

Both solution are fine for me, but from the provider point of view it is always a bit tricky to have specificity for one language. I would prefer to have the same pattern everywhere.

If we want to go with the context approach I think we need to be extra cautious to make it explicit in the documentation how we expect providers to handle this.

Kavindu-Dodan commented 1 year ago

I am also fine with language-specific implementations to handle the shutdown (context VS shutdown method).

After going through the spec, I think we should keep the keyword MUST and redefine the requirement to align with providers. Example suggestion,

The API MUST define a mechanism to propagate shutdown request to active providers

Also, requirement section ...must call the respective shutdown function on the active provider mismatch with the provider shutdown requirement [1], which is using the keyword MAY (contradict with must call as the provider might skip this)

The provider MAY define a shutdown function.... [1]


Altogether, if we agree on this, then we should correct both API & Provider spec sections to align.

[1] - https://github.com/open-feature/spec/blob/main/specification/sections/02-providers.md#25-shutdown

toddbaert commented 1 year ago

I am also fine with language-specific implementations to handle the shutdown (context VS shutdown method).

After going through the spec, I think we should keep the keyword MUST and redefine the requirement to align with providers. Example suggestion,

The API MUST define a mechanism to propagate shutdown request to active providers

Also, requirement section ...must call the respective shutdown function on the active provider mismatch with the provider shutdown requirement [1], which is uses the keyword MAY (contradict with must call as the provider might skip this)

The provider MAY define a shutdown function.... [1]

Altogether, if we agree on this, then we should correct both API & Provider spec sections to align.

[1] - https://github.com/open-feature/spec/blob/main/specification/sections/02-providers.md#25-shutdown

I like this proposal.

Kavindu-Dodan commented 1 year ago

@toddbaert this can be closed now as you already merged https://github.com/open-feature/spec/pull/193