diff --git a/internal/filedata/document.go b/internal/filedata/document.go index 4c4f9a34..a881590a 100644 --- a/internal/filedata/document.go +++ b/internal/filedata/document.go @@ -25,14 +25,37 @@ type Document struct { Segments *map[string]ldmodel.Segment } +// ReadError indicates that one of the source files could not be read or parsed. It +// distinguishes a per-file failure from a failure to merge the files' contents. +type ReadError struct { + Err error + Path string +} + +func (e *ReadError) Error() string { + return fmt.Sprintf("%s [%s]", e.Err, e.Path) +} + +func (e *ReadError) Unwrap() error { + return e.Err +} + // ReadFile reads and parses a single data file, which may be in JSON or YAML format. func ReadFile(path string) (Document, error) { + rawData, err := os.ReadFile(path) //nolint:gosec // G304: ok to read file into variable + if err != nil { + return Document{}, wrapReadError(err) + } + return parseDocument(rawData) +} + +func wrapReadError(err error) error { + return fmt.Errorf("unable to read file: %s", err) +} + +func parseDocument(rawData []byte) (Document, error) { var data Document - var rawData []byte var err error - if rawData, err = os.ReadFile(path); err != nil { //nolint:gosec // G304: ok to read file into variable - return data, fmt.Errorf("unable to read file: %s", err) - } if detectJSON(rawData) { err = json.Unmarshal(rawData, &data) } else { diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go new file mode 100644 index 00000000..d0b56592 --- /dev/null +++ b/internal/filedata/reloader.go @@ -0,0 +1,288 @@ +package filedata + +import ( + "bytes" + "crypto/sha256" + "os" + "sync" + "time" + + "github.com/launchdarkly/go-sdk-common/v3/ldlog" +) + +const ( + // DefaultDebounceDelay is a settle window long enough to coalesce the burst of change + // notifications produced by a single file edit, and short enough to stay responsive. + DefaultDebounceDelay = 100 * time.Millisecond + // DefaultRetryDelay bounds how long a failed reload can go uncorrected when no further + // change notification arrives, for example when the failure came from reading a file + // mid-write. Reading a local file is cheap, so this can be short. + DefaultRetryDelay = time.Second +) + +// ReloaderConfig configures a Reloader. +type ReloaderConfig struct { + // Paths is the list of files to load, already resolved to absolute paths. The order is + // significant: it determines which file wins under the duplicate-key handling. + Paths []string + // DuplicateKeysHandling determines what happens when the same key appears in more than + // one file. + DuplicateKeysHandling DuplicateKeysHandling + // Loggers receives log output about reloads and failures. + Loggers ldlog.Loggers + // Apply is invoked with each successfully merged result. Calls are serialized on the + // reloader's own goroutine (or, for ReloadNow, under the same lock), so implementations + // do not need their own synchronization against other reloads. Apply and OnError must + // not call back into Close. + Apply func(MergeResult) + // OnError is invoked when a reload fails, once per distinct failure: with automatic + // retries, repeats of an identical failure do not re-invoke it (a success re-arms it). + // The error is a *ReadError when a file could not be read or parsed, or a merge error + // otherwise. The reloader logs failures itself, so implementations only need to update + // their own state. + OnError func(err error) + // DebounceDelay is how long to wait after a Trigger call for further calls to settle + // before reloading, coalescing bursts of change notifications into one reload. If zero + // or negative, each Trigger reloads immediately. + DebounceDelay time.Duration + // RetryDelay is how long to wait after a failed reload before automatically retrying, + // so that a failure observed while a file was being rewritten recovers even if no + // further change notification arrives. If zero or negative, there is no automatic + // retry. + RetryDelay time.Duration + // SkipUnchanged, if true, suppresses the Apply call when the files' raw contents are + // byte-identical to the last successfully applied contents. + SkipUnchanged bool +} + +// Reloader owns the reload cycle for a set of data files: it serializes reloads, debounces +// change signals, retains the last good result on failure (by not calling Apply), retries +// after failures, and skips no-op applications. +type Reloader struct { + cfg ReloaderConfig + triggerCh chan struct{} + // retryRequestCh asks the run goroutine to arm the retry timer without performing an + // immediate reload; it is used when a synchronous ReloadNow fails. + retryRequestCh chan struct{} + closeCh chan struct{} + closeOnce sync.Once + startOnce sync.Once + + // reloadMu serializes the actual load work between ReloadNow and the run goroutine. + reloadMu sync.Mutex + lastGoodHash []byte + lastErrorMsg string +} + +// NewReloader creates a Reloader. The caller should perform the initial load with +// ReloadNow, route change signals to Trigger, and call Close when finished. +// +// The worker goroutine starts on the first ReloadNow or Trigger call rather than here: +// a Reloader can be constructed by a component whose lifecycle never uses it (a file data +// source built as a one-shot initializer, whose interface has no Close), and construction +// alone must not leak a goroutine. +func NewReloader(cfg ReloaderConfig) *Reloader { + return &Reloader{ + cfg: cfg, + triggerCh: make(chan struct{}, 1), + retryRequestCh: make(chan struct{}, 1), + closeCh: make(chan struct{}), + } +} + +func (r *Reloader) ensureStarted() { + if r.isClosed() { + return + } + r.startOnce.Do(func() { + go r.run() + }) +} + +// ReloadNow synchronously loads the files and applies the result (or reports the failure). +// It is used for the initial load; a failure here schedules the same automatic retry as a +// failed triggered reload. +func (r *Reloader) ReloadNow() { + r.ensureStarted() + if !r.reload() && r.cfg.RetryDelay > 0 { + select { + case r.retryRequestCh <- struct{}{}: + default: + } + } +} + +// Trigger signals that the files may have changed and a reload should happen after the +// debounce delay. It never blocks; signals arriving while a reload is already pending are +// coalesced. +func (r *Reloader) Trigger() { + r.ensureStarted() + select { + case r.triggerCh <- struct{}{}: + default: + } +} + +// Close stops the reloader. It does not wait for a reload that is already in progress — +// one wedged in a blocking file read (an unresponsive network mount, for example) must not +// be able to wedge shutdown — so such a reload may still deliver its result through Apply +// or OnError shortly after Close returns; consumers must tolerate that, as they always +// have for late reloads. A reload that has not yet reached its callbacks when Close is +// called will not invoke them. +func (r *Reloader) Close() { + r.closeOnce.Do(func() { + close(r.closeCh) + }) +} + +func (r *Reloader) run() { + var debounceTimer, retryTimer *time.Timer + var debounceC, retryC <-chan time.Time + stopTimer := func(timer *time.Timer) { + if timer != nil { + timer.Stop() + } + } + defer func() { + stopTimer(debounceTimer) + stopTimer(retryTimer) + }() + + reloadAndMaybeRetry := func(isRetry bool) { + if isRetry { + r.cfg.Loggers.Debug("Retrying flag data load after earlier failure") + } else { + r.cfg.Loggers.Info("Reloading flag data after detecting a change") + } + // A pending retry is superseded by this reload: it either succeeded, or it failed + // and arms a fresh retry below. + stopTimer(retryTimer) + retryTimer, retryC = nil, nil + if !r.reload() && r.cfg.RetryDelay > 0 { + retryTimer = time.NewTimer(r.cfg.RetryDelay) + retryC = retryTimer.C + } + } + + for { + select { + case <-r.closeCh: + return + case <-r.triggerCh: + if r.cfg.DebounceDelay <= 0 { + reloadAndMaybeRetry(false) + continue + } + if debounceTimer == nil { + debounceTimer = time.NewTimer(r.cfg.DebounceDelay) + debounceC = debounceTimer.C + } else { + // Stop-then-Reset without a drain is safe under this module's Go version: + // an expired timer's tick cannot be delivered after Reset. At worst a tick + // already consumed by the debounceC case races a new Trigger, producing one + // extra serialized reload that skip-unchanged absorbs. + stopTimer(debounceTimer) + debounceTimer.Reset(r.cfg.DebounceDelay) + } + case <-r.retryRequestCh: + // A synchronous ReloadNow failed; arm the retry timer without reloading again + // immediately. An already-armed retry keeps its earlier deadline. + if retryTimer == nil { + retryTimer = time.NewTimer(r.cfg.RetryDelay) + retryC = retryTimer.C + } + case <-debounceC: + debounceTimer, debounceC = nil, nil + reloadAndMaybeRetry(false) + case <-retryC: + retryTimer, retryC = nil, nil + reloadAndMaybeRetry(true) + } + } +} + +// reload performs one full load of all configured files, returning true on success (including +// a skipped no-op application). The whole set is re-read on every reload: entries are combined +// across files in order, so a change to one file can alter which file wins for a key. +func (r *Reloader) reload() bool { + r.reloadMu.Lock() + defer r.reloadMu.Unlock() + + // A trigger already queued when Close was called can still reach here; the select in + // run() does not prioritize closeCh over triggerCh. + if r.isClosed() { + return true + } + + docs := make([]Document, 0, len(r.cfg.Paths)) + hasher := sha256.New() + for _, path := range r.cfg.Paths { + rawData, err := os.ReadFile(path) //nolint:gosec // G304: ok to read file into variable + if err != nil { + return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) + } + _, _ = hasher.Write(rawData) + _, _ = hasher.Write([]byte{0}) + doc, err := parseDocument(rawData) + if err != nil { + return r.fail(&ReadError{Err: err, Path: path}) + } + docs = append(docs, doc) + } + + merged, err := Merge(r.cfg.DuplicateKeysHandling, docs...) + if err != nil { + return r.fail(err) + } + + // Close may have happened while the files were being read; deliver nothing in that case. + if r.isClosed() { + return true + } + + // A success right after a failure must apply even when the content is unchanged since + // the last success: the consumer heard about the failure through OnError and may have + // moved to an interrupted state, and only Apply tells it things are good again. + recovering := r.lastErrorMsg != "" + r.lastErrorMsg = "" + hash := hasher.Sum(nil) + if r.cfg.SkipUnchanged && !recovering && bytes.Equal(hash, r.lastGoodHash) { + return true + } + r.lastGoodHash = hash + if r.cfg.Apply != nil { + r.cfg.Apply(merged) + } + return true +} + +func (r *Reloader) fail(err error) bool { + // Close may have happened while the files were being read; deliver nothing in that + // case, and report success so no retry is armed. + if r.isClosed() { + return true + } + // With automatic retries, a persistent failure would repeat the same log entry and the + // same callback on every attempt, so repeats of an identical failure are demoted to + // debug level and do not re-invoke OnError. Status-listening consumers therefore see + // one report per distinct failure rather than one per retry. + if err.Error() == r.lastErrorMsg { + r.cfg.Loggers.Debugf("Unable to load flags: %s", err) + return false + } + r.lastErrorMsg = err.Error() + r.cfg.Loggers.Errorf("Unable to load flags: %s", err) + if r.cfg.OnError != nil { + r.cfg.OnError(err) + } + return false +} + +func (r *Reloader) isClosed() bool { + select { + case <-r.closeCh: + return true + default: + return false + } +} diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go new file mode 100644 index 00000000..13325c76 --- /dev/null +++ b/internal/filedata/reloader_test.go @@ -0,0 +1,352 @@ +package filedata + +import ( + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/launchdarkly/go-sdk-common/v3/ldlog" + "github.com/launchdarkly/go-sdk-common/v3/ldlogtest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testTimeout = 5 * time.Second + +type reloaderFixture struct { + path string + applied chan MergeResult + errored chan error + reloader *Reloader +} + +func newReloaderFixture(t *testing.T, initialContent string, configure func(*ReloaderConfig)) *reloaderFixture { + t.Helper() + f := &reloaderFixture{ + path: filepath.Join(t.TempDir(), "data.json"), + applied: make(chan MergeResult, 100), + errored: make(chan error, 100), + } + f.write(t, initialContent) + cfg := ReloaderConfig{ + Paths: []string{f.path}, + DuplicateKeysHandling: DuplicateKeysFail, + Loggers: ldlog.NewDisabledLoggers(), + Apply: func(result MergeResult) { f.applied <- result }, + OnError: func(err error) { f.errored <- err }, + } + if configure != nil { + configure(&cfg) + } + f.reloader = NewReloader(cfg) + t.Cleanup(f.reloader.Close) + return f +} + +func (f *reloaderFixture) write(t *testing.T, content string) { + t.Helper() + require.NoError(t, os.WriteFile(f.path, []byte(content), 0600)) +} + +func (f *reloaderFixture) requireApplied(t *testing.T) MergeResult { + t.Helper() + select { + case result := <-f.applied: + return result + case <-time.After(testTimeout): + require.FailNow(t, "timed out waiting for Apply") + return MergeResult{} + } +} + +func (f *reloaderFixture) requireErrored(t *testing.T) error { + t.Helper() + select { + case err := <-f.errored: + return err + case <-time.After(testTimeout): + require.FailNow(t, "timed out waiting for OnError") + return nil + } +} + +func (f *reloaderFixture) requireQuiet(t *testing.T, duration time.Duration) { + t.Helper() + select { + case <-f.applied: + require.FailNow(t, "unexpected Apply call") + case <-f.errored: + require.FailNow(t, "unexpected OnError call") + case <-time.After(duration): + } +} + +func TestReloaderInitialLoad(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, nil) + f.reloader.ReloadNow() + result := f.requireApplied(t) + require.Len(t, result.Flags, 1) + assert.Equal(t, "flag1", result.Flags[0].Key) +} + +func TestReloaderReportsFailureAndRetainsNothing(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues"`, nil) + f.reloader.ReloadNow() + err := f.requireErrored(t) + var readErr *ReadError + require.ErrorAs(t, err, &readErr) + assert.Equal(t, f.path, readErr.Path) + f.requireQuiet(t, 50*time.Millisecond) +} + +func TestReloaderDebounceCoalescesTriggers(t *testing.T) { + // The settle window is much longer than the whole trigger burst, so that a scheduling + // stall during the burst cannot let the debounce fire early and split the reloads. + // SkipUnchanged stays off so that every reload is observable as an Apply: with it on, + // identical file contents would collapse even a completely broken debounce into a + // single Apply and the test would prove nothing. + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, func(cfg *ReloaderConfig) { + cfg.DebounceDelay = 400 * time.Millisecond + }) + f.write(t, `{"flagValues": {"flag1": false}}`) + for i := 0; i < 20; i++ { + f.reloader.Trigger() + time.Sleep(time.Millisecond) + } + f.requireApplied(t) + // The burst must coalesce into one reload, with a tolerance of one more: a debounce + // tick racing a fresh trigger can legitimately produce a single extra serialized + // reload, but anything beyond that means triggers are not being coalesced. + extraApplies := 0 + deadline := time.After(600 * time.Millisecond) + for done := false; !done; { + select { + case <-f.applied: + extraApplies++ + case <-deadline: + done = true + } + } + assert.LessOrEqual(t, extraApplies, 1, "trigger burst was not coalesced") +} + +func TestReloaderReportsIdenticalFailureOnlyOnce(t *testing.T) { + logCapture := ldlogtest.NewMockLog() + f := newReloaderFixture(t, `{"flagValues"`, func(cfg *ReloaderConfig) { + cfg.RetryDelay = 10 * time.Millisecond + cfg.Loggers = logCapture.Loggers + }) + f.reloader.ReloadNow() + f.requireErrored(t) + + // The automatic retries keep failing identically; the error is neither re-reported to + // OnError nor re-logged at error level. + f.requireQuiet(t, 200*time.Millisecond) + assert.Len(t, logCapture.GetOutput(ldlog.Error), 1) + + // A different failure is a new report. + f.write(t, `{"flagValues": {bad}}`) + err := f.requireErrored(t) + assert.Contains(t, err.Error(), "error parsing file") + + // Success re-arms reporting: the same failure recurring afterward is reported again. + f.write(t, `{"flagValues": {"flag1": true}}`) + f.requireApplied(t) + f.write(t, `{"flagValues"`) + f.reloader.Trigger() + f.requireErrored(t) +} + +func TestReloaderUnusedSpawnsNoGoroutine(t *testing.T) { + // A Reloader can be constructed by a component whose lifecycle never uses or closes it, + // such as a file data source built as a one-shot initializer, so construction alone + // must not start the worker goroutine. + before := runtime.NumGoroutine() + path := filepath.Join(t.TempDir(), "data.json") + require.NoError(t, os.WriteFile(path, []byte(`{}`), 0600)) + for range 50 { + NewReloader(ReloaderConfig{ + Paths: []string{path}, + DuplicateKeysHandling: DuplicateKeysFail, + Loggers: ldlog.NewDisabledLoggers(), + Apply: func(MergeResult) {}, + }) + } + // The count is process-global, so goroutines from earlier tests may still be winding + // down; poll until it settles rather than asserting a single sample. + deadline := time.Now().Add(testTimeout) + for runtime.NumGoroutine() > before+2 { + if time.Now().After(deadline) { + require.FailNow(t, "constructing unused Reloaders must not accumulate goroutines") + } + time.Sleep(10 * time.Millisecond) + } +} + +func TestReloaderCloseDoesNotWaitForInFlightReload(t *testing.T) { + applyEntered := make(chan struct{}) + applyRelease := make(chan struct{}) + path := filepath.Join(t.TempDir(), "data.json") + require.NoError(t, os.WriteFile(path, []byte(`{"flagValues": {"flag1": true}}`), 0600)) + goroutinesBefore := runtime.NumGoroutine() + + r := NewReloader(ReloaderConfig{ + Paths: []string{path}, + DuplicateKeysHandling: DuplicateKeysFail, + Loggers: ldlog.NewDisabledLoggers(), + Apply: func(MergeResult) { + close(applyEntered) + <-applyRelease + }, + }) + + r.Trigger() + select { + case <-applyEntered: + case <-time.After(testTimeout): + require.FailNow(t, "timed out waiting for the reload to start") + } + + // The reload is parked inside Apply; Close must return anyway, because a reload wedged + // in blocking I/O must not be able to wedge shutdown. + closed := make(chan struct{}) + go func() { + r.Close() + close(closed) + }() + select { + case <-closed: + case <-time.After(testTimeout): + require.FailNow(t, "Close blocked on an in-flight reload") + } + + // Once the parked reload is released, the worker goroutine terminates rather than + // lingering past Close. Polled inline: spawning goroutines to poll would disturb the + // count being measured. + close(applyRelease) + deadline := time.Now().Add(testTimeout) + for runtime.NumGoroutine() > goroutinesBefore+1 { + if time.Now().After(deadline) { + require.FailNow(t, "worker goroutine did not exit after Close") + } + time.Sleep(10 * time.Millisecond) + } +} + +func TestReloaderRetriesAfterFailureWithoutFurtherTriggers(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, func(cfg *ReloaderConfig) { + cfg.RetryDelay = 20 * time.Millisecond + }) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, `{"flagValues"`) + f.reloader.Trigger() + f.requireErrored(t) + + // Fix the file without triggering; only the automatic retry can observe the fix. + f.write(t, `{"flagValues": {"flag1": false}}`) + f.requireApplied(t) +} + +func TestReloaderStopsRetryingAfterSuccess(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues"`, func(cfg *ReloaderConfig) { + cfg.RetryDelay = 10 * time.Millisecond + cfg.SkipUnchanged = true + }) + f.reloader.ReloadNow() + f.requireErrored(t) + + f.write(t, `{"flagValues": {"flag1": true}}`) + f.requireApplied(t) + + // After the successful reload there are no further attempts (a retry would re-apply or + // re-error; with SkipUnchanged set, a stray retry of identical content stays silent, so + // also verify via a changed file that no reload happens without a trigger). + f.write(t, `{"flagValues": {"flag1": false}}`) + f.requireQuiet(t, 100*time.Millisecond) +} + +func TestReloaderSkipUnchanged(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, func(cfg *ReloaderConfig) { + cfg.SkipUnchanged = true + }) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.reloader.Trigger() + f.requireQuiet(t, 100*time.Millisecond) + + f.write(t, `{"flagValues": {"flag1": false}}`) + f.reloader.Trigger() + f.requireApplied(t) +} + +func TestReloaderRecoveryAppliesEvenWhenContentUnchanged(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, func(cfg *ReloaderConfig) { + cfg.SkipUnchanged = true + }) + f.reloader.ReloadNow() + f.requireApplied(t) + + // A reload fails; consumers hear OnError and may move to an interrupted state. + require.NoError(t, os.Remove(f.path)) + f.reloader.Trigger() + f.requireErrored(t) + + // The files come back with byte-identical content. The success must be applied despite + // SkipUnchanged, because only Apply tells the consumer the interruption is over. + f.write(t, `{"flagValues": {"flag1": true}}`) + f.reloader.Trigger() + f.requireApplied(t) + + // Once recovered, identical content skips again. + f.reloader.Trigger() + f.requireQuiet(t, 100*time.Millisecond) +} + +func TestReloaderAppliesEveryReloadWhenSkipUnchangedIsOff(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.reloader.Trigger() + f.requireApplied(t) +} + +func TestReloaderMergesMultipleFilesInOrder(t *testing.T) { + dir := t.TempDir() + path1 := filepath.Join(dir, "first.json") + path2 := filepath.Join(dir, "second.json") + require.NoError(t, os.WriteFile(path1, []byte(`{"flagValues": {"flag1": "first"}}`), 0600)) + require.NoError(t, os.WriteFile(path2, []byte(`{"flagValues": {"flag1": "second"}}`), 0600)) + + applied := make(chan MergeResult, 10) + r := NewReloader(ReloaderConfig{ + Paths: []string{path1, path2}, + DuplicateKeysHandling: DuplicateKeysIgnoreAllButFirst, + Loggers: ldlog.NewDisabledLoggers(), + Apply: func(result MergeResult) { applied <- result }, + }) + t.Cleanup(r.Close) + r.ReloadNow() + + select { + case result := <-applied: + require.Len(t, result.Flags, 1) + case <-time.After(testTimeout): + require.FailNow(t, "timed out waiting for Apply") + } +} + +func TestReloaderDoesNothingAfterClose(t *testing.T) { + f := newReloaderFixture(t, `{"flagValues": {"flag1": true}}`, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.reloader.Close() + f.reloader.Close() // idempotent + f.reloader.Trigger() + f.requireQuiet(t, 50*time.Millisecond) +} diff --git a/ldfiledata/file_data_source_impl.go b/ldfiledata/file_data_source_impl.go index 466471a4..1966fec8 100644 --- a/ldfiledata/file_data_source_impl.go +++ b/ldfiledata/file_data_source_impl.go @@ -17,6 +17,7 @@ type fileDataSource struct { absFilePaths []string duplicateKeysHandling DuplicateKeysHandling reloaderFactory ReloaderFactory + reloader *filedata.Reloader loggers ldlog.Loggers isInitialized bool readyCh chan<- struct{} @@ -25,9 +26,6 @@ type fileDataSource struct { // closeReloaderCh is created up front rather than when the reloader starts, so that // Close never races with Start assigning it. closeReloaderCh chan struct{} - // reloaderStarted means reload calls may now come from the reloader, which is worth a - // log line; the initial load is not. - reloaderStarted bool } func newFileDataSourceImpl( @@ -52,6 +50,26 @@ func newFileDataSourceImpl( closeReloaderCh: make(chan struct{}), } fs.loggers.SetPrefix("FileDataSource:") + + // Debouncing and automatic retries only matter when something can trigger further + // reloads; a source configured without a reloader loads exactly once. Like + // closeReloaderCh, the Reloader is created up front so that Close never races an + // assignment made in Start. + var debounceDelay, retryDelay time.Duration + if reloaderFactory != nil { + debounceDelay = filedata.DefaultDebounceDelay + retryDelay = filedata.DefaultRetryDelay + } + fs.reloader = filedata.NewReloader(filedata.ReloaderConfig{ + Paths: fs.absFilePaths, + DuplicateKeysHandling: filedata.DuplicateKeysHandling(fs.duplicateKeysHandling), + Loggers: fs.loggers, + Apply: fs.applyData, + OnError: fs.handleError, + DebounceDelay: debounceDelay, + RetryDelay: retryDelay, + SkipUnchanged: true, + }) return fs, nil } @@ -61,7 +79,7 @@ func (fs *fileDataSource) IsInitialized() bool { func (fs *fileDataSource) Start(closeWhenReady chan<- struct{}) { fs.readyCh = closeWhenReady - fs.reload() + fs.reloader.ReloadNow() // If there is no reloader, then we signal readiness immediately regardless of whether the // data load succeeded or failed. @@ -71,60 +89,33 @@ func (fs *fileDataSource) Start(closeWhenReady chan<- struct{}) { } // If there is a reloader, and if we haven't yet successfully loaded data, then the - // readiness signal will happen the first time we do get valid data (in reload). - fs.reloaderStarted = true - err := fs.reloaderFactory(fs.absFilePaths, fs.loggers, fs.reload, fs.closeReloaderCh) + // readiness signal will happen the first time we do get valid data (in applyData). + err := fs.reloaderFactory(fs.absFilePaths, fs.loggers, fs.reloader.Trigger, fs.closeReloaderCh) if err != nil { fs.loggers.Errorf("Unable to start reloader: %s\n", err) } } -// Reload tells the data source to immediately attempt to reread all of the configured source files -// and update the feature flag state. If any file cannot be loaded or parsed, the flag state will not -// be modified. -func (fs *fileDataSource) reload() { - if fs.reloaderStarted { - fs.loggers.Info("Reloading flag data after detecting a change") +func (fs *fileDataSource) applyData(merged filedata.MergeResult) { + storeData := []ldstoretypes.Collection{ + {Kind: datakinds.Features, Items: merged.Flags}, + {Kind: datakinds.Segments, Items: merged.Segments}, } - docs := make([]filedata.Document, 0) - for _, path := range fs.absFilePaths { - doc, err := filedata.ReadFile(path) - if err == nil { - docs = append(docs, doc) - } else { - fs.loggers.Errorf("Unable to load flags: %s [%s]", err, path) - fs.dataSourceUpdates.UpdateStatus(interfaces.DataSourceStateInterrupted, - interfaces.DataSourceErrorInfo{ - Kind: interfaces.DataSourceErrorKindInvalidData, - Message: err.Error(), - Time: time.Now(), - }) - return - } - } - merged, err := filedata.Merge(filedata.DuplicateKeysHandling(fs.duplicateKeysHandling), docs...) - if err == nil { - storeData := []ldstoretypes.Collection{ - {Kind: datakinds.Features, Items: merged.Flags}, - {Kind: datakinds.Segments, Items: merged.Segments}, - } - if fs.dataSourceUpdates.Init(storeData) { - fs.signalStartComplete(true) - fs.dataSourceUpdates.UpdateStatus(interfaces.DataSourceStateValid, interfaces.DataSourceErrorInfo{}) - } - } else { - fs.dataSourceUpdates.UpdateStatus(interfaces.DataSourceStateInterrupted, - interfaces.DataSourceErrorInfo{ - Kind: interfaces.DataSourceErrorKindInvalidData, - Message: err.Error(), - Time: time.Now(), - }) - } - if err != nil { - fs.loggers.Error(err) + if fs.dataSourceUpdates.Init(storeData) { + fs.signalStartComplete(true) + fs.dataSourceUpdates.UpdateStatus(interfaces.DataSourceStateValid, interfaces.DataSourceErrorInfo{}) } } +func (fs *fileDataSource) handleError(err error) { + fs.dataSourceUpdates.UpdateStatus(interfaces.DataSourceStateInterrupted, + interfaces.DataSourceErrorInfo{ + Kind: interfaces.DataSourceErrorKindInvalidData, + Message: err.Error(), + Time: time.Now(), + }) +} + func (fs *fileDataSource) signalStartComplete(succeeded bool) { fs.readyOnce.Do(func() { fs.isInitialized = succeeded @@ -135,9 +126,15 @@ func (fs *fileDataSource) signalStartComplete(succeeded bool) { } // Close is called automatically when the client is closed. +// +// Close does not wait for an in-flight reload, so a late applyData/handleError can still +// reach dataSourceUpdates after Close returns. That is safe today because the SDK's status +// broadcaster serializes Broadcast against its own Close with a lock; if that Broadcaster +// discipline ever changes, this ordering needs revisiting. func (fs *fileDataSource) Close() (err error) { fs.closeOnce.Do(func() { close(fs.closeReloaderCh) + fs.reloader.Close() }) return nil } diff --git a/ldfiledatav2/file_data_source_impl.go b/ldfiledatav2/file_data_source_impl.go index daec3a50..c4d1d70b 100644 --- a/ldfiledatav2/file_data_source_impl.go +++ b/ldfiledatav2/file_data_source_impl.go @@ -3,7 +3,6 @@ package ldfiledatav2 import ( "context" "errors" - "fmt" "sync/atomic" "time" @@ -27,16 +26,15 @@ type fileDataSource struct { absFilePaths []string duplicateKeysHandling DuplicateKeysHandling reloaderFactory ReloaderFactory + reloader *filedata.Reloader loggers ldlog.Loggers // closeReloaderCh is created up front rather than when the reloader starts, so that // Close never races with Sync assigning it. closeReloaderCh chan struct{} - // reloaderStarted means reload calls may now come from the reloader, which is worth a - // log line; the initial load is not. - reloaderStarted bool - closed atomic.Bool - quit chan struct{} + closed atomic.Bool + syncStarted atomic.Bool + quit chan struct{} } func newFileDataSourceImpl( @@ -62,6 +60,26 @@ func newFileDataSourceImpl( quit: make(chan struct{}), } fs.loggers.SetPrefix("FileDataSource:") + + // Debouncing and automatic retries only matter when something can trigger further + // reloads; a source configured without a reloader loads exactly once. Like + // closeReloaderCh, the Reloader is created up front so that Close never races an + // assignment made in Sync, and a repeated Sync cannot orphan an earlier instance. + var debounceDelay, retryDelay time.Duration + if reloaderFactory != nil { + debounceDelay = filedata.DefaultDebounceDelay + retryDelay = filedata.DefaultRetryDelay + } + fs.reloader = filedata.NewReloader(filedata.ReloaderConfig{ + Paths: fs.absFilePaths, + DuplicateKeysHandling: filedata.DuplicateKeysHandling(fs.duplicateKeysHandling), + Loggers: fs.loggers, + Apply: fs.applyData, + OnError: fs.handleError, + DebounceDelay: debounceDelay, + RetryDelay: retryDelay, + SkipUnchanged: true, + }) return fs, nil } @@ -70,6 +88,17 @@ func (fs *fileDataSource) Name() string { } func (fs *fileDataSource) Sync(ds subsystems.DataSelector) <-chan subsystems.DataSynchronizerResult { + // Sync starts the reloader and the file watcher and registers listeners for this + // call's result loop, so it can run at most once: a repeat would accumulate another + // watcher and listener pair feeding nothing. The rejection channel is buffered so an + // unread rejection cannot block. + if fs.closed.Load() || !fs.syncStarted.CompareAndSwap(false, true) { + rejected := make(chan subsystems.DataSynchronizerResult, 1) + rejected <- subsystems.DataSynchronizerResult{State: interfaces.DataSourceStateOff} + close(rejected) + return rejected + } + resultChan := make(chan subsystems.DataSynchronizerResult) changeSetChan := fs.changeSetBroadcaster.AddListener() @@ -79,23 +108,23 @@ func (fs *fileDataSource) Sync(ds subsystems.DataSelector) <-chan subsystems.Dat State: interfaces.DataSourceStateInitializing, } - if fs.closed.Load() { - result.State = interfaces.DataSourceStateOff - resultChan <- result - close(resultChan) - return resultChan - } + fs.reloader.ReloadNow() - fs.reload() if fs.reloaderFactory != nil { - fs.reloaderStarted = true - err := fs.reloaderFactory(fs.absFilePaths, fs.loggers, fs.reload, fs.closeReloaderCh) + err := fs.reloaderFactory(fs.absFilePaths, fs.loggers, fs.reloader.Trigger, fs.closeReloaderCh) if err != nil { fs.loggers.Errorf("Unable to start reloader: %s\n", err) - result.State = interfaces.DataSourceStateOff - resultChan <- result - close(resultChan) - return resultChan + // With no reloader there will never be another reload; stop the worker and + // detach this call's listeners so nothing accumulates behind them. The result + // must go on a buffered channel: the reader goroutine below was never started, + // so a send on resultChan would block forever. + fs.reloader.Close() + fs.changeSetBroadcaster.RemoveListener(changeSetChan) + fs.statusBroadcaster.RemoveListener(statusChan) + rejected := make(chan subsystems.DataSynchronizerResult, 1) + rejected <- subsystems.DataSynchronizerResult{State: interfaces.DataSourceStateOff} + close(rejected) + return rejected } } @@ -134,7 +163,19 @@ func (fs *fileDataSource) Sync(ds subsystems.DataSelector) <-chan subsystems.Dat } func (fs *fileDataSource) Fetch(ds subsystems.DataSelector, ctx context.Context) (*subsystems.Basis, bool, error) { - changeSet, err := fs.load() + docs := make([]filedata.Document, 0, len(fs.absFilePaths)) + for _, path := range fs.absFilePaths { + doc, err := filedata.ReadFile(path) + if err != nil { + return nil, false, &filedata.ReadError{Err: err, Path: path} + } + docs = append(docs, doc) + } + merged, err := filedata.Merge(filedata.DuplicateKeysHandling(fs.duplicateKeysHandling), docs...) + if err != nil { + return nil, false, err + } + changeSet, err := fs.makeChangeSet(merged) if err != nil { return nil, false, err } @@ -144,69 +185,35 @@ func (fs *fileDataSource) Fetch(ds subsystems.DataSelector, ctx context.Context) }, false, nil } -// Reload tells the data source to immediately attempt to reread all of the configured source files -// and update the feature flag state. If any file cannot be loaded or parsed, the flag state will not -// be modified. -func (fs *fileDataSource) reload() { - if fs.reloaderStarted { - fs.loggers.Info("Reloading flag data after detecting a change") - } - - changeSet, err := fs.load() +func (fs *fileDataSource) applyData(merged filedata.MergeResult) { + changeSet, err := fs.makeChangeSet(merged) if err == nil { fs.changeSetBroadcaster.Broadcast(*changeSet) } else { - fs.loggers.Errorf("Unable to load flags: %s", err) - errorKind := interfaces.DataSourceErrorKindInvalidData - var readErr *fileReadError - if errors.As(err, &readErr) { - errorKind = interfaces.DataSourceErrorKindUnknown - } - fs.statusBroadcaster.Broadcast(interfaces.DataSynchronizerStatus{ - State: interfaces.DataSourceStateInterrupted, - Error: interfaces.DataSourceErrorInfo{ - Kind: errorKind, - StatusCode: 0, - Message: err.Error(), - Time: time.Time{}, - }, - FallbackToFDv1: false, - }) + fs.handleError(err) } } -// fileReadError distinguishes a failure to read or parse one of the source files from a -// failure to merge their contents. -type fileReadError struct { - err error - path string -} - -func (e *fileReadError) Error() string { - return fmt.Sprintf("%s [%s]", e.err, e.path) -} - -func (e *fileReadError) Unwrap() error { - return e.err -} - -// load synchronously reads and merges all of the configured source files, returning the -// result as a full-transfer change set. -func (fs *fileDataSource) load() (*subsystems.ChangeSet, error) { - docs := make([]filedata.Document, 0) - for _, path := range fs.absFilePaths { - doc, err := filedata.ReadFile(path) - if err != nil { - return nil, &fileReadError{err: err, path: path} - } - docs = append(docs, doc) - } - - merged, err := filedata.Merge(filedata.DuplicateKeysHandling(fs.duplicateKeysHandling), docs...) - if err != nil { - return nil, err +func (fs *fileDataSource) handleError(err error) { + errorKind := interfaces.DataSourceErrorKindInvalidData + var readErr *filedata.ReadError + if errors.As(err, &readErr) { + errorKind = interfaces.DataSourceErrorKindUnknown } + fs.statusBroadcaster.Broadcast(interfaces.DataSynchronizerStatus{ + State: interfaces.DataSourceStateInterrupted, + Error: interfaces.DataSourceErrorInfo{ + Kind: errorKind, + StatusCode: 0, + Message: err.Error(), + Time: time.Time{}, + }, + FallbackToFDv1: false, + }) +} +// makeChangeSet expresses a merged file data set as a full-transfer change set. +func (fs *fileDataSource) makeChangeSet(merged filedata.MergeResult) (*subsystems.ChangeSet, error) { intent := subsystems.ServerIntent{ Payload: subsystems.Payload{ ID: "", @@ -242,6 +249,7 @@ func (fs *fileDataSource) Close() (err error) { if swapped := fs.closed.CompareAndSwap(false, true); swapped { close(fs.quit) close(fs.closeReloaderCh) + fs.reloader.Close() return nil // already closed } diff --git a/ldfiledatav2/file_data_source_test.go b/ldfiledatav2/file_data_source_test.go index 210944ed..068a6d3f 100644 --- a/ldfiledatav2/file_data_source_test.go +++ b/ldfiledatav2/file_data_source_test.go @@ -2,7 +2,9 @@ package ldfiledatav2 import ( "context" + "errors" "os" + "runtime" "testing" "time" @@ -12,6 +14,7 @@ import ( "github.com/launchdarkly/go-server-sdk/v7/subsystems" th "github.com/launchdarkly/go-test-helpers/v3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSuccessfullyLoadsJsonFlags(t *testing.T) { @@ -234,7 +237,16 @@ func TestRecoversFromInvalidDataWhenFileUpdated(t *testing.T) { reloadCh <- struct{}{} - result = <-resultChan + // The failed initial load schedules automatic retries, so additional interrupted + // results may arrive before the reload that observes the corrected file. + deadline := time.After(5 * time.Second) + for result.State != interfaces.DataSourceStateValid { + select { + case result = <-resultChan: + case <-deadline: + require.FailNow(t, "timed out waiting for a valid result") + } + } assert.NotNil(t, result.ChangeSet) assert.Len(t, result.ChangeSet.Changes(), 1) assert.Equal(t, "my-flag", result.ChangeSet.Changes()[0].Key) @@ -343,3 +355,74 @@ func TestCloseStopsReloader(t *testing.T) { } }) } + +func TestInitializerBuildAndFetchLeaksNoGoroutines(t *testing.T) { + th.WithTempFileData([]byte(`{"flags": {"my-flag": {"on": true}}}`), func(filename string) { + goroutinesBefore := runtime.NumGoroutine() + + // A DataInitializer has no Close and the data system never tears one down, so + // building and fetching must not start anything that outlives the calls. + for i := 0; i < 20; i++ { + initializer, err := DataSource().FilePaths(filename).AsInitializer().Build(subsystems.BasicClientContext{}) + assert.NoError(t, err) + _, _, err = initializer.Fetch(mocks.NewMockDataSelector(subsystems.NoSelector()), context.Background()) + assert.NoError(t, err) + } + + assert.LessOrEqual(t, runtime.NumGoroutine(), goroutinesBefore+2, + "initializer builds must not accumulate goroutines") + }) +} + +func TestSyncReturnsOffWhenReloaderFactoryFails(t *testing.T) { + th.WithTempFileData([]byte(`{"flags": {"my-flag": {"on": true}}}`), func(filename string) { + f := func(paths []string, loggers ldlog.Loggers, reload func(), closeCh <-chan struct{}) error { + return errors.New("no watches available") + } + + factory := DataSource().FilePaths(filename).Reloader(f) + sync, err := factory.Build(subsystems.BasicClientContext{}) + assert.NoError(t, err) + defer sync.Close() + + // The failure must surface as an Off result rather than a hang. + results := make(chan subsystems.DataSynchronizerResult, 1) + go func() { + resultChan := sync.Sync(mocks.NewMockDataSelector(subsystems.NoSelector())) + results <- <-resultChan + _, stillOpen := <-resultChan + assert.False(t, stillOpen, "result channel should be closed after the rejection") + }() + select { + case result := <-results: + assert.Equal(t, interfaces.DataSourceStateOff, result.State) + case <-time.After(5 * time.Second): + require.FailNow(t, "Sync hung after the reloader factory failed") + } + }) +} + +func TestSecondSyncIsRejected(t *testing.T) { + th.WithTempFileData([]byte(`{"flags": {"my-flag": {"on": true}}}`), func(filename string) { + factory := DataSource().FilePaths(filename) + sync, err := factory.Build(subsystems.BasicClientContext{}) + assert.NoError(t, err) + defer sync.Close() + + resultChan := sync.Sync(mocks.NewMockDataSelector(subsystems.NoSelector())) + result := <-resultChan + assert.Equal(t, interfaces.DataSourceStateValid, result.State) + + // Sync is single-use: a repeat is rejected with a single Off result and a closed + // channel, without disturbing the first Sync. + secondChan := sync.Sync(mocks.NewMockDataSelector(subsystems.NoSelector())) + select { + case second := <-secondChan: + assert.Equal(t, interfaces.DataSourceStateOff, second.State) + case <-time.After(5 * time.Second): + require.FailNow(t, "second Sync produced no result") + } + _, stillOpen := <-secondChan + assert.False(t, stillOpen, "second Sync's channel should be closed") + }) +}