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

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

Closed
toddbaert opened this issue May 24, 2023 · 5 comments
Closed

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

toddbaert opened this issue May 24, 2023 · 5 comments
Assignees

Comments

@toddbaert
Copy link
Member

toddbaert commented May 24, 2023

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
Copy link
Contributor

weyert commented May 24, 2023

I think that's reasonable

@thomaspoignant
Copy link
Member

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
Copy link
Contributor

Kavindu-Dodan commented May 24, 2023

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
Copy link
Member Author

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
Copy link
Contributor

@toddbaert this can be closed now as you already merged #193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants