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] Evaluate feature flag from synchronous code (no async/await support) causes deadlock #149

Closed
DouglasHammon-FV opened this issue Sep 15, 2023 · 6 comments · Fixed by #150
Labels
bug Something isn't working

Comments

@DouglasHammon-FV
Copy link
Contributor

Observed behavior

Hello. I been working on a custom feature flag solution for our company. It is custom tailed to fit our use cases and needs. I came across OpenFeature a few months back and found it well thought-out, so we adopted it as a standard. Kudos on doing this work.

We have implemented a custom provider that uses our internal feature flag solution. We have many software projects that need to evaluate feature flags and some of them do not support async/await. When I try and call GetBooleanDetails on IFeatureClient I call GetAwaiter().GetResult() at the end to force the code to support synchronous evaluation. This causes a deadlock. I have ensured that our custom provide calls ConfigureAwait(false) on all async calls. Looking at the source code, it appears FeatureClient.GetBooleanDetails awaits Tasks from inner methods, but ConfigureAwait(false) is not called.

My assumption is that because none of the awaited calls with FeatureClient have ConfigureAwait(false), it's causing the deadlock because the calling code tries to resume execution on the same thread (which fails in an MVC application).

Expected Behavior

Calling .GetAwaiter().GetResult() or .Result should not cause a deadlock, but force the code to execute synchronously.

Steps to reproduce

  • Configure OpenFeature API with a provider that has async calls.
  • Execute: client.GetBooleanDetails(flagKey, false, customContext).Result; within an ASP.Net MVC (4.8 framework) action that does not support async/await or return a Task<>
  • Observe that the request never completes and a deadlock is created.
@DouglasHammon-FV DouglasHammon-FV added the bug Something isn't working label Sep 15, 2023
@beeme1mr
Copy link
Member

Hey @DouglasHammon-FV, thanks for reaching out. I'm sorry to hear that you ran into this issue. Would this be something you would be interested in helping to fix?

FYI, @toddbaert @bacherfl @benjiro @kinyoklion

@DouglasHammon-FV
Copy link
Contributor Author

@beeme1mr Yes, I would be willing to submit an PR. Do the maintainers agree that the proper fix is to add ConfigureAwait(false) to all async calls?

@benjiro
Copy link
Member

benjiro commented Sep 18, 2023

@DouglasHammon-FV please feel free to submit the PR. This was just an oversight probably on my part we definitely should add ConfigureAwait(false).

We probably need to have a further discussion of whether we provide a sync API as well, so avoid the overhead of sync over async.

@DouglasHammon-FV
Copy link
Contributor Author

@beeme1mr @benjiro I've created PR 150. I've confirmed it fixes the issue I'm seeing. Please review.

@toddbaert
Copy link
Member

toddbaert commented Sep 19, 2023

I think unless we do expose a dedicated sync API, we should see if we can add a lint rule or some other kind of enforcement to help us remember to configure this for maximum compatibility. I'll investigate that, or at least make an issue.

I have little experience with .NET-MVC or other frameworks with such caveats and totally overlooked this as well.

@benjiro
Copy link
Member

benjiro commented Sep 19, 2023

@toddbaert Yea there is an analyzer rule for this https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2007 We can just enable it as an error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants