refactor!: Improve EventExecutor #393
Conversation
…d signatures Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…t processing Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…o separate methods Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…ed new expressions Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…l cases Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…ider directly Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
- Coverage 86.16% 85.92% -0.25%
==========================================
Files 39 39
Lines 1605 1606 +1
Branches 173 173
==========================================
- Hits 1383 1380 -3
- Misses 186 189 +3
- Partials 36 37 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
askpt
requested review from
beeme1mr,
benjiro,
kinyoklion,
austindrenski and
toddbaert
askpt
changed the title
There was a problem hiding this comment.
PR Overview
This PR refactors the EventExecutor and related classes for improved code readability, performance, and enhanced null safety. Key changes include replacing explicit constructor calls with array initializer syntax for collections, switching from Thread to Task-based asynchronous processing, and adding null-safety checks on EventChannel usages.
Reviewed Changes
| File | Description |
|---|---|
| src/OpenFeature/EventExecutor.cs | Refactored to use Task.Run instead of Thread, updated collection initializations, and improved event handling. |
| test/OpenFeature.Tests/Providers/Memory/InMemoryProviderTests.cs | Added null-forgiving operator to EventChannel usage in tests. |
| src/OpenFeature/Providers/Memory/InMemoryProvider.cs | Added null checks before writing to the EventChannel to improve null safety. |
| test/OpenFeature.Tests/TestImplementations.cs | Updated EventChannel usage with null-forgiving operator. |
| src/OpenFeature/FeatureProvider.cs | Updated EventChannel to be nullable for enhanced null safety. |
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
| message = "Provider is ready"; | ||
| } | ||
| else if (status == ProviderStatus.Error && eventType == ProviderEventTypes.ProviderError) | ||
| var message = status switch |
| } | ||
| } | ||
|
|
||
| private void ProcessClientHandlers(Event e) |
There was a problem hiding this comment.
You removed a comment, but I guess the method name makes it clear what's happening now, so I'm fine with it.
|
|
||
| private ILogger _logger; | ||
|
|
||
| public EventExecutor() | ||
| { | ||
| this._logger = NullLogger<EventExecutor>.Instance; | ||
| var eventProcessing = new Thread(this.ProcessEventAsync); | ||
| eventProcessing.Start(); | ||
| Task.Run(this.ProcessEventAsync); |
There was a problem hiding this comment.
I should have caught this earlier.
|
@askpt could you explain the breaking change? How could the event channel be null if we set it? I must be missing something. I guess this would force providers to do a null check, right? I guess this would only break providers? I won't have any impact on consumers will it? I don't think we want to release a v3 for this. |
This PR
This pull request includes several changes to the
EventExecutorclass and related classes in theOpenFeatureproject to improve code readability and performance. The main changes include replacing the use ofListandDictionaryconstructors with array initializers, refactoring methods to useTaskinstead ofThread, and enhancing null safety for theEventChannel.Codebase improvements:
src/OpenFeature/EventExecutor.cs: ReplacedListandDictionaryconstructors with array initializers, refactored methods to useTaskinstead ofThread, and improved method readability and maintainability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Null safety enhancements:
src/OpenFeature/FeatureProvider.cs: MadeEventChannelnullable to enhance null safety and updated related methods accordingly. [1] [2]src/OpenFeature/Providers/Memory/InMemoryProvider.cs: Added null check forEventChannelbefore writing to it.test/OpenFeature.Tests/Providers/Memory/InMemoryProviderTests.cs: Added null-forgiving operator toGetEventChannelmethod call.test/OpenFeature.Tests/TestImplementations.cs: Added null-forgiving operator toEventChannelusage.Related Issues
Reference #358
Notes
This PR fixes a null reference annotation in the
FeatureProvider. This change could potentially break the external providers, but it is necessary since theEventChannelcould be null.