Repository navigation
Make structs thread-safe #233
Description
Background: #229
I've pushed a few changes that make the chain struct thread-safe:
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 racetarget which runs all tests under race detector - add corresponding step to CI
- create issue for implementing stress test
Activity
Rohith-Raju commentedon Jan 25, 2023
@gavv I'd like to take this up. Sounds interesting!!
gavv commentedon Jan 25, 2023
Great!
gavv commentedon Jan 26, 2023
gavv commentedon Jan 26, 2023
gavv commentedon Jan 29, 2023
@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 commentedon Feb 23, 2023
#293 (comment)
gavv commentedon Feb 23, 2023
6c92b92
sujeetsawala commentedon Apr 28, 2023
Is this still open?
gavv commentedon Apr 28, 2023
@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 commentedon Apr 28, 2023
@gavv I have the following questions:-
Can you explain the objectives of our stress test?