Don't care about already released TestThreadContexts

[304NotModified]

🤔 What's changed?

Ignore this error, as nothing goes wrong after it.

⚡️ What's your motivation?

Fixes https://github.com/reqnroll/Reqnroll/issues/387

Also, there is another "Container was already taken by another thread" comment, see https://github.com/reqnroll/Reqnroll/blob/966d1c372bdef843d7a2e3ecd267e07722351d80/Reqnroll/TestRunnerManager.cs#L186

🏷️ What kind of change is this?

• :bug: Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

📋 Checklist:

• [x] I've changed the behaviour of the code
  • [ ] I have added/updated tests to cover my changes.
• [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
• [x] Users should know about my change
  • [x] I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[aravind666]

@304NotModified or @gasparnagy , Is there a roadmap or plan to merge this, We are seeing this issue since 2.0.4 version, Please kindly let us know as we are blocked to upgrade to any of the versions release till date from 2.0.4

[304NotModified]

Hi @aravind666 this needs a review. I think this is a good idea to merge, but that is only my opinion and also I don't have the rights to merge this PR.

[304NotModified]

@obligaron tagging you because of #144

[aravind666]

Thank you so much for your kind response @304NotModified , @obligaron Please kindly let us know when is the plan to merge this.

[obligaron]

Sorry for the delay.

The interesting question is why a TestThreadContext is released twice.

ReleaseTestRunner is only called under xUnit after the end of each scenario (via IAsyncLifetime.DisposeAsync).

If the error occurs, either DisposeAsync must be called several times or there must be an error in the TestThreadContext management. The former would be an error in the test framework, the latter an error in Reqnroll.

With the current PR, we would not throw an exception for either variant. This means that, in the worst case, we would cover up other errors in Reqnroll.

We could explicitly solve both cases differently if necessary. We could set the testRunner to zero in the generated code for DisposeAsync and then do nothing when DisposeAsync is called again. If it was an error / borderline case in xUnit, the exception would be resolved. If it was an error in Reqnroll, we would still get the exception and have more clarity (of course it would be even better to reproduce the error in a sample application).

What do you think? If you are interested, I can create a PR for the code change.

[304NotModified]

Sounds good to me!

[obligaron]

I opened #580 with the described approach. 🙂

[gasparnagy]

Let's keep this open and see if #580 fixes #387. If the issue will still persist, we can apply this "ultimate fix".

Also, there is another "Container was already taken by another thread" comment

This is part of normal flow, not exceptional case. Basically we let multiple threads to enter the critical section and the first one will win and the other will retry. So this comment is not related to the issue IMHO.

[304NotModified]

I'm curious what Copilot things (FYI I fine with dropping this PR)

[gasparnagy]

@304NotModified Did you get what you expected from Copilot? I'm surprised that it did not mention anything for the merge conflict.

[gasparnagy]

Let's close this. We can restore it if needed.