Make structs thread-safe

[gavv]

Background: #229

I've pushed a few changes that make the chain struct thread-safe:

• 0547acd
• a961221
• 90675e1

This automatically makes many structs thread-safe too: Expect, Value, Array, Object, etc. It's true for matcher structs which contains only a chain + immutable data.

The next step is to manually implement thread-safety for the following structs:

• Environment
• Request
• Websocket

These three structs has mutable state and should be protected manually. For each of them, we should add sync.RWMutex field and protect all operations. We can remove noCopy fields from these structs.

Request struct needs special care: it contains transforms and matchers callbacks that are invoked during Expect() call. We should NOT call them under a lock because otherwise it would be easy to write code with deadlocks (if you use the same request from this callbacks).

Websocket also needs some special handling. It should allow to read and write messages concurrently and to call disconnect concurrently (similarly as gorilla websocket which it uses under the hood).

After finishing this, we should update docs:

• add thread-safety section to package documentation, mention limitation placed by httpexpect, testify, and standard testing
• add note to README

In addition, we should improve our testing suite:

• add make race target which runs all tests under race detector
• add corresponding step to CI
• create issue for implementing stress test

[Rohith-Raju]

@gavv I'd like to take this up. Sounds interesting!!

[gavv]

Great!

[gavv]

@Rohith-Raju

I was reviewing #254 and realized that there is a problem with our approach.

We acquire a lock, then perform a check, and then report failure, under the lock. A potential problem is that reporting failure means invoking AssertionHandler, Reporter, Logger, Formatter, which may be implemented by user. In other words, we invoke user code under a lock.

If this user code will try to access our object, we'll get deadlock. For example, if AssertionHandler tries to get a value from env, env method will try to acquire a lock and will hang.

I have an idea of quite generic approach to avoid this. We can introduce three rules:

• every operation starts with entering chain; this rule is already followed

• if operation needs a lock, it's acquired after entering chain and released before leaving chain

• when fail is called, it records failure but doesn't report it; it will be reported only when leave is called

This way, failures are guaranteed to be reported outside of the lock (because leave is called after releasing lock). Which means that associated user code is also called outside of the lock.

Two simple changes are needed:

• in your pr, just reorder enter/leave block and lock/unlock block in every method

• modify chain to postpone failure reporting from fail() to leave(); if possible please do it in a separate pr and we'll merge it first

[gavv]

#293 (comment)

[gavv]

6c92b92

[sujeetsawala]

Is this still open?

[gavv]

@sujeetsawala yes. I plan to finish it by myself, except stress test. If you would like to help with stress test, you're welcome! It can be done independently of other stuff.

[sujeetsawala]

@gavv I have the following questions:-

1. A multi-process or a single-process?
2. Should it be generalised so that functions to be stress tested can be injected into or a particular for this.
3. I was looking at the stress go library (https://pkg.go.dev/golang.org/x/tools/cmd/stress) which provides the above functionalities out of the box.

Can you explain the objectives of our stress test?