Azure / Azure/azure-sdk-for-cpp
Add a thread sanitizer CI lane and Event Hubs thread safety tests
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## Summary
The Event Hubs library has no thread safety test and no sanitizer lane in CI. The races in #7284 thus went into `main` with no warning, and a new race can do the same. A thread sanitizer build and a small test suite let CI hold the thread safety guarantee.
Parent: #7284. Severity tags follow the key in #7252. The code anchors refer to commit `2de7c583e`.
## Motivation
A thread sanitizer finds all three defects in #7284. It needs one test that shares a client across threads. No such test exists. The stress test applies load (`test/eventhubs-stress-test/`). It does not share one client across threads, and it does not run under a sanitizer.
A sanitizer lane also protects the locking rework, which is the largest change in this workstream. #7286 changes `EnsureSender` to a double-checked write. #7288 moves to a `std::shared_mutex`. A small error in either change gives a rare race, not a test failure.
The repository can already run an ASan lane or a UBSan lane. A TSan job can then extend it. Read the CI definitions before you add a job.
## Proposal
Build support.
- [ ] Add a CMake option that turns on `-fsanitize=thread` for clang and gcc. **[GA quality bar]**
- [ ] Put ASan and UBSan on the same option. The option selects one sanitizer, because a binary cannot hold the thread sanitizer and the address sanitizer together. **[GA quality bar]**
- [ ] Add a CI job that builds the Event Hubs tests with the thread sanitizer and runs the thread safety tests. **[GA quality bar]**
Tests.
- [ ] Add a test for the unlocked map read. N threads call `Send` to N different partitions that no thread used before. The test uses one shared `ProducerClient`. **[GA quality bar]**
- [ ] Add a shared-client stress test. One `ProducerClient`, many threads, sends to the same partition and to different partitions. The test counts the messages that arrive. **[GA quality bar]**
- [ ] Add a wrong-use test. A `TryAdd` during a `Send` on the same batch fails each time, and the batch stays correct. **[GA quality bar]**
- [ ] Add a processor test. Parallel partition handlers write to a checkpoint store. The test loses no checkpoint and duplicates no checkpoint. **[GA quality bar]**
- [ ] Add a lifecycle test for the races in #7255: a parallel `Send` and `Close`, then a parallel `Start` and `Stop`. The two workstreams then share one suite. **[GA quality bar]**
- [ ] Extend the stress test to apply load to one shared client (`test/eventhubs-stress-test/`). **[Post GA]**
The map-read test must cause a thread sanitizer error before #7285. It must pass after #7285.
The tests that need a broker must use the existing test broker, not a live namespace. The lane then stays cheap enough for every pull request. #7270 pinned the broker hash, so the target is stable.
## Validation
- [ ] The thread sanitizer lane runs in CI. A reported race fails the build.
- [ ] The map-read test causes a thread sanitizer error when you revert #7285.
- [ ] The full test suite passes under the thread sanitizer with no suppressions. A necessary suppression has a comment that gives the reason and the issue that removes it.
- [ ] The lane is fast enough to run on every pull request.
Contributor guide
Assessment
This issue has not been assessed yet.