Skip to content
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

[BUG] EventExecutor exception #358

Closed
askpt opened this issue Jan 31, 2025 · 4 comments
Closed

[BUG] EventExecutor exception #358

askpt opened this issue Jan 31, 2025 · 4 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed tests Improvement or additions to test suite

Comments

@askpt
Copy link
Member

askpt commented Jan 31, 2025

Description

We are currently experiencing some flaky unit tests that we should tackle. Example output:

Unhandled exception. System.Threading.Channels.ChannelClosedException: The channel has been closed.
   at OpenFeature.EventExecutor.ProcessFeatureProviderEventsAsync(Object providerRef) in /_/src/OpenFeature/EventExecutor.cs:line 238
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
The active test run was aborted. Reason: Test host process crashed : Unhandled exception. System.Threading.Channels.ChannelClosedException: The channel has been closed.
   at OpenFeature.EventExecutor.ProcessFeatureProviderEventsAsync(Object providerRef) in /_/src/OpenFeature/EventExecutor.cs:line 238
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()


Test Run Aborted.
Total tests: Unknown
     Passed: 90
 Total time: 2.8570 Seconds
     6>_VSTestConsole:
         MSB4181: The "VSTestTask" task returned false but did not log an error.
     6>Done Building Project "/home/runner/work/dotnet-sdk/dotnet-sdk/test/OpenFeature.Tests/OpenFeature.Tests.csproj" (VSTest target(s)) -- FAILED.

As part of the debugging, we can try using the dotnet test—- blame to understand what is wrong.

Notes

@askpt askpt added bug Something isn't working help wanted Extra attention is needed tests Improvement or additions to test suite labels Jan 31, 2025
@kinyoklion
Copy link
Member

This looks to be a bug in the EventExecutor rather than just a flaky test. Writing to a channel will throw if the channel has been closed.

There is a purple note about it here: https://learn.microsoft.com/en-us/dotnet/core/extensions/channels#bounding-strategies

There isn't any synchronization between shutdown and the event processing loop.

            while (await reader.WaitToReadAsync().ConfigureAwait(false))
            {
                if (!reader.TryRead(out var item))
                    continue;

//!!!!!!!!!!!!!!! If you are executing any of the following, and ShutDownAsync is called or a providerr is registered, then you will get an exception.
                switch (item)
                {
                    case ProviderEventPayload eventPayload:
                        this.UpdateProviderStatus(typedProviderRef, eventPayload);
                        await this.EventChannel.Writer.WriteAsync(new Event { Provider = typedProviderRef, EventPayload = eventPayload }).ConfigureAwait(false);
                        break;
                }
            }

Theoretically this could happen in real situations and not jsut a test.

It seems the typical pattern is for the producer to close the channel after it has exhausted things it wants to produce.

So, typically I think that writes and completing the channel would be happening either on the same thread, or would be synchronized.

In our case though I think we could just handle the exception from the WriteAsync.

@askpt askpt changed the title [BUG] Flaky Unit Tests [BUG] EventExecutor exception Feb 24, 2025
@askpt
Copy link
Member Author

askpt commented Feb 24, 2025

This looks to be a bug in the EventExecutor rather than just a flaky test. Writing to a channel will throw if the channel has been closed.

There is a purple note about it here: https://learn.microsoft.com/en-us/dotnet/core/extensions/channels#bounding-strategies

There isn't any synchronization between shutdown and the event processing loop.

            while (await reader.WaitToReadAsync().ConfigureAwait(false))
            {
                if (!reader.TryRead(out var item))
                    continue;

//!!!!!!!!!!!!!!! If you are executing any of the following, and ShutDownAsync is called or a providerr is registered, then you will get an exception.
                switch (item)
                {
                    case ProviderEventPayload eventPayload:
                        this.UpdateProviderStatus(typedProviderRef, eventPayload);
                        await this.EventChannel.Writer.WriteAsync(new Event { Provider = typedProviderRef, EventPayload = eventPayload }).ConfigureAwait(false);
                        break;
                }
            }

Theoretically this could happen in real situations and not jsut a test.

It seems the typical pattern is for the producer to close the channel after it has exhausted things it wants to produce.

So, typically I think that writes and completing the channel would be happening either on the same thread, or would be synchronized.

In our case though I think we could just handle the exception from the WriteAsync.

@kinyoklion Thanks for investigating this. I changed the title and description to better align with your findings.

@askpt askpt self-assigned this Feb 28, 2025
@askpt askpt linked a pull request Feb 28, 2025 that will close this issue
@askpt askpt removed a link to a pull request Feb 28, 2025
@askpt
Copy link
Member Author

askpt commented Feb 28, 2025

I created this PR #393 to start cleaning up and fixing the bug in the EventExecutor. I will fix the bug in the following PR.

toddbaert pushed a commit that referenced this issue Mar 12, 2025
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

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`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL13-R28):
Replaced `List` and `Dictionary` initializations with shorthand syntax
`[]`.
[[1]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL13-R28)
[[2]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL45-R41)
[[3]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL73-R75)
*
[`src/OpenFeature/EventExecutor.cs`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL149-R144):
Changed from using `Thread` to `Task` for asynchronous operations to
improve performance and simplify the code.
[[1]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL149-R144)
[[2]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL219-R213)
[[3]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL237-R257)

Refactoring for better clarity and maintainability:

*
[`src/OpenFeature/EventExecutor.cs`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL189-L204):
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`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cR266-R275):
Split `ProcessEventAsync` method into smaller methods
(`ProcessApiHandlers`, `ProcessClientHandlers`,
`ProcessDefaultProviderHandlers`) for better organization and
maintainability.
[[1]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cR266-R275)
[[2]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL281-R298)
[[3]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL305-R311)
*
[`src/OpenFeature/EventExecutor.cs`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL219-R213):
Converted `ProcessFeatureProviderEventsAsync` and `ProcessEventAsync`
from `void` to `Task` to follow best practices for asynchronous methods.
[[1]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL219-R213)
[[2]](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL237-R257)

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Reference #358

---------

Signed-off-by: André Silva <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Mar 18, 2025
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

This pull request includes changes to the `EventChannel` handling in the
`OpenFeature` library to improve code consistency and reliability.

Improvements to `EventChannel` handling:

*
[`src/OpenFeature/EventExecutor.cs`](diffhunk://#diff-f563baadcf750bb66efaea76d7ec4783320e6efdc45c468effb531c948a2292cL150-R150):
Removed unnecessary null-conditional operator when completing the
`Writer` of the event channel.
*
[`src/OpenFeature/FeatureProvider.cs`](diffhunk://#diff-96ebc8fc507d0a19d55b9a5cb57b72a0e8058e09f31ee7d0b39e99b00c5029d8L143-R143):
Made the `GetEventChannel` method non-virtual to ensure consistent
behavior across different implementations.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Related to #358

---------

Signed-off-by: André Silva <[email protected]>
@askpt
Copy link
Member Author

askpt commented Apr 7, 2025

@toddbaert, @kinyoklion, @beeme1mr It seems after PR #393 the build tests are now stabilized. Can we close this issue?

@beeme1mr beeme1mr closed this as completed Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed tests Improvement or additions to test suite
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants