SpecFlowOSS
Question: Container thread safety
Hi @gasparnagy !
I wonder if BoDi is thread-safe.
From source code it seems that BoDi is not thread safe and not support multiple parallel Resolve(..).
But Specflow is passing BoDi containers pretty freely. It's up to caller to prevent reentrancy?
If yes, let's document it
Hi @leotsarev!
Yes, you are right, probably it should be made more clear that it is not thread safe.
SpecFlow manages 4 level of contexts (and the related containers): global, test-thread, feature, scenario. The parallel execution of tests happen on the test-thread level (ie. if SpecFlow realizes that the test runner is about to execute a test on a new thread, it creates a test-thread context for it). The feature and scenario container are sub-contexts of the test-thread container, therefore they are also explicitly attached to one logical execution thread. This means, that (unless you spawn new threads within your step definition), test-thread, feature and scenario containers are not used from multiple threads.
The global context (and the related global container) is a shared resource, so therefore it is accessible from multiple threads, therefore if anyone storing any state there, the thread-safety has to be ensured somehow. The good news is that for storing global state, people can also use a simple static field so they are rarely use the global container for that.
So definitely the problem is there, but it only affects very special cases. Saying that I am happy if anyone improves the documentation regarding that, but somehow we have to highlight that for the "usual" cases (when people use test-thread, feature or scenario containers and not have multiple threads created from the binding code) the manual synchronization is not necessary.
Hi @gasparnagy ! Thanks you for this project and for your answers.
I opened PR with small comment changes #28
However, I still wondering if this is optimal solution. We now battling some thread-safety issue in Specflow, and seems that other people have this problem to: #24 (by @Greybird). I wonder if fixing it on container level (at least for Resolve) will really lead to performance degradation or code complexity.
Another question: Could you please elaborate on
unless you spawn new threads within your step definition
What is spawn new threads? Will async/await lead to problems? What we should avoid in our code to evade thread problems?
Or this questions is better suited for Specflow repository?
Another question. Assume that X is registered in parent container per-container, and two different child containers will simultaneously try to resolve X from different threads. Should it work?
In my practice, it happened when Specflow tries to perform two ScenarioInitialize in parallel.
Hi @leotsarev .
I can confirm I have to run on a modified BoDi version with Specflow and NUnit to activate parallellism due to https://github.com/SpecFlowOSS/SpecFlow/issues/657.
This issue is due to a flaw in the Specflow/BoDi integration with regards to RuntimePlugins.
To me the simpler way is to make BoDi thread safe, and this really won't hurt IMHO, with very minimal cost, even if my #24 PR might not be the best approach for long term maintainability.
Would you be able to clarify if you are open to integrate thread-safety in BoDi, or if this will not happen, so that we can focus on alternate solutions if needed?
Thanks,
Would you be able to clarify if you are open
I'm a user of Specflow, just like you :-) That's question better suited for @gasparnagy, probably
That's question better suited for @gasparnagy, probably
Sorry, this was the intention, but I now realize my sentence was unclear 😄
@leotsarev @Greybird Thank you for your comments.
Two clarifications:
- running the scenario async is normally not a problem, because even though there will be multiple threads allocated, there will be a single logical execution thread anyway (unless you kick off multiple parallel async tasks that use the scenario context)
- If a type is registered in the global container than the object will be populated there, so parallel resolve operations from multiple sub-containers (e.g. scenario container) is definitely an issue. If the type is registered in test-thread container or below, there will be (normally) anyway one one active child, so that does not cause issues.
Related to make BoDi thread safe: I agree with you. Most probably it would not cause performance degradation. (Would be good to measure it tough.) If you feel having energy for that, I would be happy to help with reviews and comments. Fortunately the functionality is quite well covered with unit tests, so this refactoring can be done more or less in a safe way. If you make a few tests that show some of the parallel issues (we will have no chance to cover all issues, but at least 3-4 typical cases that we are aware of), than we can get a better confidence about the thread safety plus maybe we can use the same tests as a some sort of performance benchmark. (Maybe we could first duplicate the entire BoDi code file to a different namespace, so we can "easily" switch between the versions used by the tests.
For now, I figured that our problems with Specflow was caused by misconfiguration on our side. :-) So I have no immediate pressure to fix something in BoDi, but if you think that some of suggestions/PoC is worth pursuing, I'll like to contiribute.
@Greybird that's really stupid error https://github.com/SpecFlowOSS/SpecFlow/issues/1948 so I don't think it would be your case :-) We struggled with it, because it reproduced only on one machine, classic symptom of both misconfiguration and thread safety :-)