gavv
Make structs thread-safe
Background: https://github.com/gavv/httpexpect/discussions/229
I've pushed a few changes that make the chain struct thread-safe:
- 0547acd58636b8dc199f43fd7206d9bb629e7852
- a96122195174a5b9df2804e41c8abe92a90fc52b
- 90675e11729f5dfa5121bbf41a1e071dbf9e52e7
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:
- [x] Environment
- [x] 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 racetarget which runs all tests under race detector - [ ] add corresponding step to CI
- [ ] create issue for implementing stress test
I suggest to split work into five PRs:
- [x] modify chain to postpone failure reporting until leave() is called
- [x] make Environment thread-safe (easy part)
- [ ] make Request thread-safe (harder)
- [ ] make Websocket thread-safe (harder)
- [ ] add
make raceto makefile and CI - [ ] add stress test (not very hard but big and time consuming)
UPDATED
Added this paragraph to the issue:
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).
@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
@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.
@gavv I have the following questions:-
- A multi-process or a single-process?
- Should it be generalised so that functions to be stress tested can be injected into or a particular for this.
- 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?