ThreadTask: fix thread lifetime ordering and make detach flag atomic #3662
Conversation
Change: Reorder ThreadTask data members so std::thread is destroyed last, ensuring all members accessed by the running thread remain valid during shutdown. Also make the detach flag atomic to avoid potential data races between the executor thread and the owning thread. Reason: This change improves thread-safety and prevents subtle undefined behavior without modifying the public API.
|
Please can you review the changes? Thanks |
So while this is true, can you write any tests or CI checks that would fail before your change that would show improved correctness and saftey? |
|
Thanks @sbiscigl , Good question. ThreadTask isn’t referenced directly in tests, but it’s used internally by PooledThreadExecutor, so executor tests already exercise it indirectly. My plan is to add a TSAN-gated stress test to PooledThreadExecutorTest.cpp that races executor shutdown with concurrent Submit() calls to hit the detach/shutdown path. This should surface the existing race on m_detached under ThreadSanitizer and show the benefit of making it atomic. I’ll follow up once that’s in place. The std::thread reordering is a small construction-order safety improvement to avoid partially constructed state being observed by the worker. |
|
So in general when we accept PR's we want reproducible issues that are impacting customers. It is a slippery slope to change implementation of key components of the SDK without real proven impact. So from a user standpoint how could you use the SDK in way where this issue would come up? i.e. how could invoking a client API trigger this behavior and can you trigger it in a reproducible way? That is the kind of test I would want to see along this change. One that i can verify that this change is fixing something for users. |
|
@sbiscigl Thanks. |
2 participants
Issue #, if available:
N/A
Description of changes:
This PR improves the thread-safety and lifetime correctness of
ThreadTask.Changes include:
std::threadis destroyed last, ensuring allmembers accessed by the worker thread remain valid during destruction.
m_detachedtostd::atomic<bool>to prevent data races whenaccessed concurrently by multiple threads.
order and using a lambda for thread startup for improved clarity.
These changes do not modify the public API and are purely internal
correctness and safety improvements.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.