refactor: Improve EventExecutor

[askpt]

This PR

This pull request to src/OpenFeature/EventExecutor.cs includes changes to improve code readability and performance. The most important changes include replacing List and Dictionary initializations with shorthand syntax, switching from Thread to Task for asynchronous operations, and refactoring methods for better clarity and maintainability.

Improvements to code readability and performance:

• src/OpenFeature/EventExecutor.cs: Replaced List and Dictionary initializations with shorthand syntax []. [1] [2] [3]
• src/OpenFeature/EventExecutor.cs: Changed from using Thread to Task for asynchronous operations to improve performance and simplify the code. [1] [2] [3]

Refactoring for better clarity and maintainability:

• src/OpenFeature/EventExecutor.cs: Refactored EmitOnRegistration method to use a switch expression for setting the message based on provider status and event type, improving readability.
• src/OpenFeature/EventExecutor.cs: Split ProcessEventAsync method into smaller methods (ProcessApiHandlers, ProcessClientHandlers, ProcessDefaultProviderHandlers) for better organization and maintainability. [1] [2] [3]
• src/OpenFeature/EventExecutor.cs: Converted ProcessFeatureProviderEventsAsync and ProcessEventAsync from void to Task to follow best practices for asynchronous methods. [1] [2]

Related Issues

Reference #358

[codecov]

Codecov Report

Attention: Patch coverage is 87.95181% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.90%. Comparing base (9b6feab) to head (e2c1f3a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/OpenFeature/EventExecutor.cs	87.95%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
- Coverage   86.16%   85.90%   -0.27%     
==========================================
  Files          39       39              
  Lines        1605     1603       -2     
  Branches      173      172       -1     
==========================================
- Hits         1383     1377       -6     
- Misses        186      189       +3     
- Partials       36       37       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

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.

[toddbaert]

Good improvements, thanks.

[toddbaert]

@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.

[askpt]

@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.

@toddbaert If you check test OpenFeature_Should_Shutdown_Unused_Provider you will see the test will fail if you don't do a null check in the method StartListeningAndShutdownOld, particularly on the line oldProvider.GetEventChannel()?.Writer.Complete();. It seems there is the possibility for the EventChannel to be null in this case, so it needs to be marked as a nullable. This is why I needed to mark it as null and the potential breaking change (even though it would be at compile time).

[toddbaert]

@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.

@toddbaert If you check test OpenFeature_Should_Shutdown_Unused_Provider you will see the test will fail if you don't do a null check in the method StartListeningAndShutdownOld, particularly on the line oldProvider.GetEventChannel()?.Writer.Complete();. It seems there is the possibility for the EventChannel to be null in this case, so it needs to be marked as a nullable. This is why I needed to mark it as null and the potential breaking change (even though it would be at compile time).

Isn't that just because we use Substitute.For in that test to create these providers?

What if we remove the virtual from the GetEventChannel accessor though? So it cannot be overridden... that might be a way to guarantee it cannot be null in any case, right? I think that technically still might be breaking (in case somebody overrode it) but it seems more correct and more ergonomic, and also "less breaking".

[askpt]

@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.

@toddbaert If you check test OpenFeature_Should_Shutdown_Unused_Provider you will see the test will fail if you don't do a null check in the method StartListeningAndShutdownOld, particularly on the line oldProvider.GetEventChannel()?.Writer.Complete();. It seems there is the possibility for the EventChannel to be null in this case, so it needs to be marked as a nullable. This is why I needed to mark it as null and the potential breaking change (even though it would be at compile time).

Isn't that just because we use Substitute.For in that test to create these providers?

What if we remove the virtual from the GetEventChannel accessor though? So it cannot be overridden... that might be a way to guarantee it cannot be null in any case, right? I think that technically still might be breaking (in case somebody overrode it) but it seems more correct and more ergonomic, and also "less breaking".

@toddbaert That is a good point. Let's do this. I removed the breaking change part, and we can discuss it on another topic. This PR is already complex enough. Plus, I need to investigate it better to be able to reply to your points! 👍

[Copilot]

Pull Request Overview

This pull request refactors the EventExecutor to improve readability and performance by adopting shorthand collection initializers, switching from Thread to Task for asynchronous operations, and reorganizing event processing methods. Additional changes include emitting events in the in-memory provider and removing duplicate test helper methods.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/OpenFeature/EventExecutor.cs	Refactored initialization with shorthand syntax, replaced Thread with Task, and reorganized event processing.
src/OpenFeature/Providers/Memory/InMemoryProvider.cs	Updated the flag update event emission using the EventChannel.
test/OpenFeature.Tests/TestImplementations.cs	Removed duplicate SendEventAsync method to simplify test implementations.

Comments suppressed due to low confidence (2)

src/OpenFeature/EventExecutor.cs:18

• Using '[]' for List initialization is unconventional in C#. It is recommended to use 'new()' to maintain clarity and avoid potential compilation issues.

private readonly List<FeatureProvider> _activeSubscriptions = [];

src/OpenFeature/EventExecutor.cs:20

• The shorthand syntax '[]' for initializing a Dictionary may not be supported in all C# versions. Consider using 'new()' for better readability and consistency.

private readonly Dictionary<ProviderEventTypes, List<EventHandlerDelegate>> _apiHandlers = [];