Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890213002:
Prevent data race in GetStaticInstance (Closed)
DescriptionPrevent data race in GetStaticInstance
The previous code attempted to lock instance_count and instance with a
CriticalSection, but the CriticalSection was not static, so each
function invocation got its own instance. Locking this call-specific
instance doesn't actually stop any other threads from concurrently
accessing the same function-scope globals, so this function had a data
race, which broke tsan tests (and possibly other things).
Making the CriticalSection shared among function calls will actually
synchronize access to the globals and allow our tsan tests to pass.
BUG=webrtc:3062
Review-Url: https://codereview.webrtc.org/2890213002
Cr-Commit-Position: refs/heads/master@{#18296}
Committed: https://chromium.googlesource.com/external/webrtc/+/c66f8d7d6d3d39987927348818a1b6e50f786cf7
Patch Set 1 #
Messages
Total messages: 20 (10 generated)
The CQ bit was checked by cstanfill@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
cstanfill@google.com changed reviewers: + mflodman@webrtc.org
Hi magnus, I think I have a fix for a data race issue in some old code (triggered tsan presubmit failures downstream). Not sure if I'm introducing more problems by adding in a nontrivial function-static variable though. Let me know what you want to do with this (added it to an old webrtc bug that looked relevant but I could make a new one)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Prevent data race in GetStaticInstance The previous code attempted to lock instance_count and instance with a CriticalSection, but the CriticalSection was not static, so each function invocation got its own instance. Locking this call-specific instance doesn't actually stop any other threads from concurrently accessing the same function-scope globals, so this function had a data race, which broke tsan tests (and possibly other things). Making the CriticalSection shared among function calls will actually synchronize access to the globals and allow our tsan tests to pass. BUG=webrtc:3062 ========== to ========== Prevent data race in GetStaticInstance The previous code attempted to lock instance_count and instance with a CriticalSection, but the CriticalSection was not static, so each function invocation got its own instance. Locking this call-specific instance doesn't actually stop any other threads from concurrently accessing the same function-scope globals, so this function had a data race, which broke tsan tests (and possibly other things). Making the CriticalSection shared among function calls will actually synchronize access to the globals and allow our tsan tests to pass. BUG=webrtc:3062 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org, nisse@webrtc.org - mflodman@webrtc.org
Modified reviewers to owners in webrtc/system_wrappers.
lgtm
friendly ping to kwiberg@, i think this needs your lgtm
lgtm However, since static locals are initialized in a thread-safe way since C++11, I guess this entire function could be removed if someone took the time. See e.g. https://stackoverflow.com/questions/11711920/how-to-implement-multithread-saf...
On 2017/05/24 08:59:14, kwiberg-webrtc wrote: > lgtm > > However, since static locals are initialized in a thread-safe way since C++11, I > guess this entire function could be removed if someone took the time. See e.g. > https://stackoverflow.com/questions/11711920/how-to-implement-multithread-saf... Thanks! I think that's probably true; the bugs I was looking through before writing the CL implied a lot of people wanted to get rid of it. BTW it looks like I am not a webrtc committer so could one of the reviewers on this CL please land it through the commit queue?
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/05/25 14:47:43, cstanfill wrote: > On 2017/05/24 08:59:14, kwiberg-webrtc wrote: > > lgtm > > > > However, since static locals are initialized in a thread-safe way since C++11, > I > > guess this entire function could be removed if someone took the time. See e.g. > > > https://stackoverflow.com/questions/11711920/how-to-implement-multithread-saf... > > Thanks! > I think that's probably true; the bugs I was looking through before writing the > CL implied a lot of people wanted to get rid of it. > BTW it looks like I am not a webrtc committer so could one of the reviewers on > this CL please land it through the commit queue? I've pushed the button.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1496040175620210, "parent_rev":
"3a343ad32983c8f5731400889cb72622c06c7337", "commit_rev":
"c66f8d7d6d3d39987927348818a1b6e50f786cf7"}
Message was sent while issue was closed.
Description was changed from ========== Prevent data race in GetStaticInstance The previous code attempted to lock instance_count and instance with a CriticalSection, but the CriticalSection was not static, so each function invocation got its own instance. Locking this call-specific instance doesn't actually stop any other threads from concurrently accessing the same function-scope globals, so this function had a data race, which broke tsan tests (and possibly other things). Making the CriticalSection shared among function calls will actually synchronize access to the globals and allow our tsan tests to pass. BUG=webrtc:3062 ========== to ========== Prevent data race in GetStaticInstance The previous code attempted to lock instance_count and instance with a CriticalSection, but the CriticalSection was not static, so each function invocation got its own instance. Locking this call-specific instance doesn't actually stop any other threads from concurrently accessing the same function-scope globals, so this function had a data race, which broke tsan tests (and possibly other things). Making the CriticalSection shared among function calls will actually synchronize access to the globals and allow our tsan tests to pass. BUG=webrtc:3062 Review-Url: https://codereview.webrtc.org/2890213002 Cr-Commit-Position: refs/heads/master@{#18296} Committed: https://chromium.googlesource.com/external/webrtc/+/c66f8d7d6d3d3998792734881... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/c66f8d7d6d3d3998792734881...
Message was sent while issue was closed.
Thank you! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
