Skip to content

feat!: Poll and use realtime at the same time #153

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

Draft
wants to merge 1 commit into
base: feat/event-channel
Choose a base branch
from

Conversation

rolodato
Copy link
Member

This PR is based on top of #151, but kept separate for easier reviewing.

Breaking changes

WithRealtime no longer disables polling

Before, using WithRealtime would prevent polling from happening, so that the SDK's only method of updating the environment is be via SSE. There was no way of using SSE combined with polling. This is undesirable for several reasons:

  • SSE is an optional Flagsmith component that can fail just like the Flagsmith API can. Since SSE still needs the API to function, it makes sense to leverage the Flagsmith API as an alternative source via polling if SSE is unavailable. This should be handled transparently by the SDK and not require manual intervention.
  • SSE does not have message delivery guarantees; it's not possible to tell whether messages have been missed for any reason. Even if we're mainly using SSE for updates, it can still be useful to set a less frequent polling interval to have some guarantee that the SDK is not running with a too outdated environment.

To disable polling, use WithEnvironmentRefreshInterval(0).

NewClient now returns an error if the environment could not be initialised

Previously, NewClient would always succeed and then try to initialise the environment in the background. If this initialisation fails and is unrecoverable (e.g. network is unavailable and we're not working offline), users would have no way of catching this besides logs.

Now, if NewClient needs to fetch an environment to initialise, it now blocks until the initial environment is fetched or an error is returned. The default timeout is 10 seconds, or can be changed with WithTimeout.

New features

When Client polls for environment updates, it now checks if it already has a recent environment (i.e. younger than the polling interval) before making requests.

Real-time updates can now be used together with polling.

WithEnvironmentRefreshInterval(0) now prevents polling from starting. If realtime is also disabled, NewClient now returns an error.

Added Client.Close. This lets customer applications cleanly terminate the goroutines created by NewClient.

@rolodato rolodato marked this pull request as ready for review March 25, 2025 02:33
@gagantrivedi
Copy link
Member

Now, if NewClient needs to fetch an environment to initialise, it now blocks until the initial environment is fetched

I remember someone complaining about this behaviour because now the whole application has to wait for Flagsmith to fetch the environment document, which was understandably a big no-no for them.

@rolodato
Copy link
Member Author

Interesting, I can see how that could be a problem. I actually really like LaunchDarkly's solution for this and wouldn't mind outright copying it: https://pkg.go.dev/github.com/launchdarkly/go-server-sdk/v7#MakeClient

Pretty much every use case is accounted for: timeouts, initial/recoverable/permanent failures, and async initialisation

We could add a WithInitialisationTimeout option which defaults to the request timeout. And if NewClient fails, it always returns a Client instance that can at least be used with the default flag handler.

@rolodato rolodato mentioned this pull request Mar 26, 2025
@rolodato rolodato marked this pull request as draft March 27, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants