-
Notifications
You must be signed in to change notification settings - Fork 8
chore: Add FDv2 Data System #196
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
Conversation
…ctional-data-source-updates
…rlamb/sdk-1584/fdv2-polling-data-source
…rlamb/sdk-1584/fdv2-polling-data-source
…rlamb/sdk-1584/fdv2-polling-data-source
…-1589/data-system-configurtion
…-1589/data-system-configurtion
…-1589/data-system-configurtion
|
bugbot review |
| if (!ApplyToLegacyPersistence(changeSet)) | ||
| { | ||
| // TODO: Probably throw? | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Silent failure when applying to legacy persistent store
When ApplyToLegacyPersistence returns false (indicating a failure to write to a legacy non-transactional persistent store), the failure is silently ignored. The data has already been written to the memory store at this point, so the memory and persistent stores become inconsistent. On SDK restart, the persistent store would have stale data. The failure result from ApplyToLegacyPersistence is not propagated or handled, leaving only a TODO comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be addressed later.
Co-authored-by: Todd Anderson <127344469+tanderson-ld@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Task hangs forever if disposed during StartCurrent
The StartCurrent method checks _disposed outside the lock, then creates a TaskCompletionSource and enqueues an action to complete it. If Dispose is called between the check and the action being processed, InternalDispose clears _pendingActions, orphaning the TaskCompletionSource. The returned Task will never complete, causing any caller awaiting it to hang indefinitely. The EnqueueAction method doesn't check disposal state before enqueueing, allowing actions to be added and then immediately discarded.
pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs#L239-L282
dotnet-core/pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs
Lines 239 to 282 in 7af16ce
| /// </summary> | |
| public Task<bool> StartCurrent() | |
| { | |
| if (_disposed) | |
| { | |
| return Task.FromResult(false); | |
| } | |
| var tcs = new TaskCompletionSource<bool>(); | |
| EnqueueAction(() => | |
| { | |
| IDataSource dataSourceToStart; | |
| lock (_lock) | |
| { | |
| TryFindNextUnderLock(); | |
| dataSourceToStart = _currentDataSource; | |
| } | |
| if (dataSourceToStart is null) | |
| { | |
| // No sources available. | |
| tcs.SetResult(false); | |
| return; | |
| } | |
| // Start the source asynchronously and complete the task when it finishes. | |
| // We do this outside the lock to avoid blocking the queue. | |
| _ = Task.Run(async () => | |
| { | |
| try | |
| { | |
| var result = await dataSourceToStart.Start().ConfigureAwait(false); | |
| tcs.TrySetResult(result); | |
| } | |
| catch (Exception ex) | |
| { | |
| tcs.TrySetException(ex); | |
| } | |
| }); | |
| }); | |
| return tcs.Task; | |
| } |
pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs#L94-L121
dotnet-core/pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs
Lines 94 to 121 in 7af16ce
| private void InternalDispose(DataSourceStatus.ErrorInfo? error = null) | |
| { | |
| // When disposing the whole composite, we bypass the action queue and tear | |
| // down the current data source immediately while still honoring the same | |
| // state transitions under the shared lock. Any queued actions become no-ops | |
| // because there is no current data source | |
| // been disconnected. | |
| lock (_lock) | |
| { | |
| // cut off all the update proxies that have been handed out first | |
| _disableableTracker.DisablePreviouslyTracked(); | |
| // dispose of the current data source | |
| _currentDataSource?.Dispose(); | |
| _currentDataSource = null; | |
| // clear any queued actions and reset processing state | |
| _pendingActions.Clear(); | |
| _isProcessingActions = false; | |
| _currentEntry = default; | |
| _disposed = true; | |
| } | |
| // report state Off directly to the original sink, bypassing the sanitizer | |
| // which would map Off to Interrupted (that mapping is only for underlying sources) | |
| _originalUpdateSink.UpdateStatus(DataSourceState.Off, error); | |
| } |
ada9d23 to
cd2491a
Compare
cd2491a to
48f4568
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Exception swallowing can cause Start() to hang indefinitely
The ProcessQueuedActions method catches and swallows all exceptions from enqueued actions. When StartCurrent() enqueues an action that calls TryFindNextUnderLock(), if the data source factory or action applier factory throws an exception, it gets silently caught. The TaskCompletionSource is never completed (no SetResult or SetException), causing the returned Task from Start() to hang indefinitely. Any caller awaiting Start() would block forever if factory initialization fails.
pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs#L177-L186
dotnet-core/pkgs/sdk/server/src/Internal/DataSources/CompositeDataSource/CompositeSource.cs
Lines 177 to 186 in dd9ec86
| // the next action to prevent one failure from stopping all processing. | |
| try | |
| { | |
| action(); | |
| } | |
| catch | |
| { | |
| // Continue processing remaining actions even if one fails | |
| // TODO: need to add logging, will add in next PR | |
| } |
Note
Adds the FDv2 data system with a new transactional update API, composite source orchestration, write-through store, and integrated streaming/polling with selector support, plus extensive tests.
IDataSourceUpdatesV2(transactional updates API) and adapts legacy sources viaDataSourceUpdatesV2ToV1Adapter.FDv2DataSystem,WriteThroughStore(memory+persistent, selector-aware), andSelectorSourceFacade.LdClientto use FDv2 whenConfiguration.DataSystemis set.CompositeSourceusingIDataSourceObserver,ObservableDataSourceUpdates, andDataSourceUpdatesSanitizer(V2).IActionApplierwith observer pattern and disableable sinks; updatesSourceFactory/ActionApplierFactorysignatures.FDv2DataSourcecomposes initializers/synchronizers with fallback/blacklist logic and timed recovery.AssemblyInfo.Written by Cursor Bugbot for commit dd9ec86. This will update automatically on new commits. Configure here.