Error reporting changes introduced in v2.5.0 are thread-unsafe. Should it be documented or addressed ?

[valsteen]

Hi,

first of all I thank contributors to this library which simplifies a lot my test scenarios, I really like the fluent interface approach for this use case.

I recently updated httpexpect from v2.3.1 to v2.8.0 and noticed that tests were crashing due to data races ( I run tests with -race ). There is no mention of thread safety in the documentation, so I'm not sure if it's intentional, it was so far a guess that using the same httpexpect.Expect instance across goroutines would not be an issue. My tests use goroutines in order to cover race condition scenarios.

I could identify that #150 , which was merged in v2.5.0, introduced this change: chain.enter is called from practically all methods, and it's updating a slice, mutatinghttpexpect.Expect in a non-thread-safe manner. I'm not sure how to address this issue, it's fair enough that I use a newhttpexpect.Expect instance in those cases - I can't call that a bug since thread safety is not mentioned in the documentation. But what would help is to clarify whether concurrent uses of the same httpexpect.Expect instance is intentionally non-thread-safe, or if it should be considered unintentional and that there is some hope that a future version would address this.

[gavv]

Hi! Thanks for report and digging into the problem.

httpexpect is not thread safe. If it was thread-safe in earlier releases, it was by accident.

However, I had thoughts on making top-level components thread-safe and documenting that (including Expect instance).

Ideally I'd make the lib thread-safe completely, but I'm not sure if making Request thread-safe would be easy: it can invoke user callbacks which may in turn directly or indirectly invoke methods of Request (because why not) and it can lead to hard-to-catch deadlocks.

I'll take a look of making chain thread-safe in upcoming days, this should be the very first step towards this direction because most components are based on it. Then I'll create an issue for making other components thread-safe; feel free to help.

[valsteen]

Thank you, yes it's probably sufficiently ergonomic to say top-level methods such as GET, POST, etc are thread-safe - if this is what it implies. My initial unconscious expectation was that I could use it just like http.Client, that's probably how most users would see it. I'll follow the issue and see if I can help - I guess we first need a bit more clarity on what should be the outcome

[gavv]

Great, thanks. Good analogy with http.Client, BTW.

[gavv]

I've pushed commits that make chain thread-safe and created an issue for remaining steps: #233

[gavv]

I've tagged v2.10.0 which includes commits that make chain thread-safe.