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

Capture ExecutionContext after every binding invoke #126

Merged
merged 1 commit into from
May 13, 2024

Conversation

obligaron
Copy link
Contributor

Fixes #120

Reqnroll (and Specflow v4,x) uses Async internally (only one code path).
To ensure the user sees no difference between the old (synchronous) implementation and the new ones the ExecutionContext (including AsyncLocal) are handled manually by Reqnroll.
This is done using ExecutionContext.Capture() in BindingDelegateInvoker,InvokeInExecutionContext.

The problem is that this only works once.
Background:

  • In the first call
    • ExecutionContextHolder.Value is null.
    • This means that ExecutionContext.Run is not called.
    • The Binding-Method is called with the current ExecutionContext and modifies the AsyncLocal.
    • Current ExecutionContext is captured
    • => everything is fine.
  • In the second call
    • ExecutionContextHolder.Value is not null.
    • ExecutionContext.Run is called.
      • ExecutionContext.Run captures the current ExecutionContext
      • In ExecutionContext.Run the Binding-Method is called and modifies the AsyncLocal.
      • The ExecutionContext.Run restores the captured "old current" ExecutionContext with the old values
    • The current (restored) ExecutionContext is captured
    • => Information is lost.

Bufix: Capture ExecutionContext explicitly in ExecutionContext.Run after every binding invoke.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea and the unit tests are great, I made a suggestion for simplifying the code.

Reqnroll/Bindings/BindingDelegateInvoker.cs Outdated Show resolved Hide resolved
@obligaron obligaron force-pushed the ExecutionContextMultipleWrite branch from 691131e to 3aff447 Compare May 13, 2024 15:00
Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@gasparnagy gasparnagy merged commit c9e4984 into reqnroll:main May 13, 2024
4 checks passed
@obligaron obligaron deleted the ExecutionContextMultipleWrite branch May 13, 2024 15:33
@gasparnagy
Copy link
Contributor

gasparnagy commented May 13, 2024

@obligaron you should be now on the @reqnroll/core-team (you need to accept invitation), so pushing new branches to the main repo should work now. (I hope. Please report if not.)

gasparnagy added a commit that referenced this pull request May 22, 2024
…ons-dependencyinjection-plugin

* origin/main: (21 commits)
  Fix #56 autofac ambiguous stepdef and hook required #127 issue (#139)
  Reduce target framework of Reqnroll to netstandard2.0 (#130)
  Fix StackOverflowException when using [StepArgumentTransformation] with same input and output type (#136)
  MsTest: Replace DelayedFixtureTearDown special case with ClassCleanupBehavior.EndOfClass (#128)
  Temporarily disabled tests until #132 is resolved
  Add NUnit & xUnit core tests to portability suite
  Capture ExecutionContext after every binding invoke (#126)
  small improvement in CodeDomHelper to be able to chain async calls
  fix method name sources in UnitTestFeatureGenerator
  External data plugin, support for JSON files  (#118)
  UnitTests: Check if SDK version is installed and if not ignore the test (#109)
  Fix 111 ignore attr not inherited from rule (#113)
  Update README.md (#110)
  Fix 81 - modified CucumberExpressionParameterTypeRegistry to handle multiple custom types used as cucumber expressions when those types share the same short name. (#104)
  Update index.md
  Simplify test project targets (#105)
  Make SystemTests temp folder configurable and use NUGET_PACKAGES env var to override global NuGet folder
  Fleshing out Generation System Tests (2) (#99)
  Fix for 81 - Cucumber Expression using Enums errors when two enums exist with the same short name (#100)
  Include BoDi to Reqnroll package (#91) (#95)
  ...

# Conflicts:
#	Reqnroll.sln
#	Tests/Reqnroll.PluginTests/Reqnroll.PluginTests.csproj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution Context (AsyncLocal) does not get transferred between [BeforeScenario] and the steps.
2 participants