Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1439613004:
Fix a data race in the thread unit tests. (Closed)
|
Created:
5 years, 1 month ago by nisse-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix a data race in the thread unit tests.
The flag used in thread_unittest.cc:FunctorB is subject to a (mostly
harmless) data race. In a tsan build, reproduce using
out/Release/rtc_unittests --gtest_filter=AsyncInvokeTest.FireAndForget
There are additional tsan warnings, not all deterministic, when
running all the rtc_unittets: Some data races related to destructors,
and a locking-order-inversion warning. Hence applying this patch does
not make the unit tests tsan-clean.
I should also add that this is my very first cl, so I'm not at all
familiar with the process.
Committed: https://crrev.com/d9b75bef5d0749ea94b31cedab0105c241938954
Cr-Commit-Position: refs/heads/master@{#10645}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Update with formatting, explicit constructor, use of CritScope. #
Total comments: 2
Patch Set 3 : Fixed member naming, make constructor explicit, fix assignment return type. #
Total comments: 4
Patch Set 4 : Addressed final(?) nits. #Messages
Total messages: 18 (4 generated)
Description was changed from ========== Fix a data race in the thread unit tests. The flag used in thread_unittest.cc:FunctorB is subject to a (mostly harmless) data race. In a tsan build, reproduce using out/Release/rtc_unittests --gtest_filter=AsyncInvokeTest.FireAndForget There are additional tsan warnings, not all deterministic, when running all the rtc_unittets: Some data races related to destructors, and a locking-order-inversion warning. Hence applying this patch does not make the unit tests tsan-clean. I should also add that this is my very first cl, so I'm not at all familiar with the process. ========== to ========== Fix a data race in the thread unit tests. The flag used in thread_unittest.cc:FunctorB is subject to a (mostly harmless) data race. In a tsan build, reproduce using out/Release/rtc_unittests --gtest_filter=AsyncInvokeTest.FireAndForget There are additional tsan warnings, not all deterministic, when running all the rtc_unittets: Some data races related to destructors, and a locking-order-inversion warning. Hence applying this patch does not make the unit tests tsan-clean. I should also add that this is my very first cl, so I'm not at all familiar with the process. ==========
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, tommi@webrtc.org
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:154: { You are not following the C++ style guide. The WebRTC style guide is essentially the same as the Google style guide: https://google.github.io/styleguide/cppguide.html I strongly recommend using 'git cl format' to make sure formatting follows the style guide. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} I'm not sure you are allowed to use implicit conversions like this. tommi wdyt? https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:158: cs.Enter(); Instead of manual Enter() and Leave(), you can use: rtc::CritScope scoped_lock(&cs);
great start! I'll wait for the next patch set. Btw, you can try running this to format the code: git cl format https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:172: bool flag; fyi - pbos is working on a change that could be helpful: https://codereview.webrtc.org/1420043008/
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:154: { On 2015/11/11 15:50:44, magjed_webrtc wrote: > You are not following the C++ style guide. The WebRTC style guide is essentially > the same as the Google style guide: > https://google.github.io/styleguide/cppguide.html > I strongly recommend using 'git cl format' to make sure formatting follows the > style guide. Done. I've also added a google style in my emacs cc-mode config. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/11 15:50:44, magjed_webrtc wrote: > I'm not sure you are allowed to use implicit conversions like this. tommi wdyt? I can make it explicit. Only reason for implicit was that I tried to minimize the changes where the flag is declared and initialized, from "bool called = false;" to "AtomicFlag called = false;". With explicit, I have to change the structure to "called(false)", which isn't a big deal, I guess. I'm also considering adding a default initial value of false, does that make sense? Then the declaration + initializer would be just "AtomicFlag called;". https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:157: bool operator= (bool value) { What's the proper return type for this assignment operator? Is it better to return a reference to *this? https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:158: cs.Enter(); On 2015/11/11 15:50:44, magjed_webrtc wrote: > Instead of manual Enter() and Leave(), you can use: > rtc::CritScope scoped_lock(&cs); Done. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:172: bool flag; On 2015/11/11 16:00:03, tommi (webrtc) wrote: > fyi - pbos is working on a change that could be helpful: > https://codereview.webrtc.org/1420043008/ I'm adding a reference to rtc::AtomicInt in the comment.
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 08:39:05, nisse wrote: > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > I'm not sure you are allowed to use implicit conversions like this. tommi > wdyt? > > I can make it explicit. Only reason for implicit was that I tried to minimize > the changes where the flag is declared and initialized, from "bool called = > false;" to "AtomicFlag called = false;". With explicit, I have to change the > structure to "called(false)", which isn't a big deal, I guess. > > I'm also considering adding a default initial value of false, does that make > sense? Then the declaration + initializer would be just "AtomicFlag called;". Adding a default ctor that initializes to false makes sense to me. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:157: bool operator= (bool value) { On 2015/11/12 08:39:05, nisse wrote: > What's the proper return type for this assignment operator? Is it better to > return a reference to *this? yes, return a reference to *this. https://codereview.webrtc.org/1439613004/diff/20001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/20001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:168: mutable CriticalSection cs; member variables have a trailing underscore. e.g. cs_ and flag_.
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:154: { On 2015/11/12 08:39:05, nisse wrote: > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > You are not following the C++ style guide. The WebRTC style guide is > essentially > > the same as the Google style guide: > > https://google.github.io/styleguide/cppguide.html > > I strongly recommend using 'git cl format' to make sure formatting follows the > > style guide. > > Done. I've also added a google style in my emacs cc-mode config. You can also take a look at go/emacs. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 08:39:05, nisse wrote: > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > I'm not sure you are allowed to use implicit conversions like this. tommi > wdyt? > > I can make it explicit. Only reason for implicit was that I tried to minimize > the changes where the flag is declared and initialized, from "bool called = > false;" to "AtomicFlag called = false;". With explicit, I have to change the > structure to "called(false)", which isn't a big deal, I guess. > > I'm also considering adding a default initial value of false, does that make > sense? Then the declaration + initializer would be just "AtomicFlag called;". FYI - Here is a link to the style guide: "Do not define implicit conversions. Use the explicit keyword for conversion operators and single-argument constructors." https://google.github.io/styleguide/cppguide.html#Implicit_Conversions tommi seems fine with this and tommi > style guide. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:157: bool operator= (bool value) { On 2015/11/12 08:39:05, nisse wrote: > What's the proper return type for this assignment operator? Is it better to > return a reference to *this? The typical copy assignment operator would take the same type as argument, i.e. AtomicFlag& operator= (const AtomicFlag& value)
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 08:49:52, tommi (webrtc) wrote: > On 2015/11/12 08:39:05, nisse wrote: > > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > > I'm not sure you are allowed to use implicit conversions like this. tommi > > wdyt? > > > > I can make it explicit. Only reason for implicit was that I tried to minimize > > the changes where the flag is declared and initialized, from "bool called = > > false;" to "AtomicFlag called = false;". With explicit, I have to change the > > structure to "called(false)", which isn't a big deal, I guess. > > > > I'm also considering adding a default initial value of false, does that make > > sense? Then the declaration + initializer would be just "AtomicFlag called;". > > Adding a default ctor that initializes to false makes sense to me. Done. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 09:20:51, magjed_webrtc wrote: > On 2015/11/12 08:39:05, nisse wrote: > > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > > I'm not sure you are allowed to use implicit conversions like this. tommi > > wdyt? > > > > I can make it explicit. Only reason for implicit was that I tried to minimize > > the changes where the flag is declared and initialized, from "bool called = > > false;" to "AtomicFlag called = false;". With explicit, I have to change the > > structure to "called(false)", which isn't a big deal, I guess. > > > > I'm also considering adding a default initial value of false, does that make > > sense? Then the declaration + initializer would be just "AtomicFlag called;". > > FYI - Here is a link to the style guide: > "Do not define implicit conversions. Use the explicit keyword for conversion > operators and single-argument constructors." > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > tommi seems fine with this and tommi > style guide. I changed the constructor to explicit (and with a default false). Should I make the conversion to bool explicit too? Then I'd have to add some conversion in the invocation of the EXPECT_TRUE_WAIT macro, which isn't too bad. https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:157: bool operator= (bool value) { On 2015/11/12 08:49:52, tommi (webrtc) wrote: > On 2015/11/12 08:39:05, nisse wrote: > > What's the proper return type for this assignment operator? Is it better to > > return a reference to *this? > > yes, return a reference to *this. Done. https://codereview.webrtc.org/1439613004/diff/20001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/20001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:168: mutable CriticalSection cs; On 2015/11/12 08:49:52, tommi (webrtc) wrote: > member variables have a trailing underscore. e.g. cs_ and flag_. Done.
https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 09:20:51, magjed_webrtc wrote: > On 2015/11/12 08:39:05, nisse wrote: > > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > > I'm not sure you are allowed to use implicit conversions like this. tommi > > wdyt? > > > > I can make it explicit. Only reason for implicit was that I tried to minimize > > the changes where the flag is declared and initialized, from "bool called = > > false;" to "AtomicFlag called = false;". With explicit, I have to change the > > structure to "called(false)", which isn't a big deal, I guess. > > > > I'm also considering adding a default initial value of false, does that make > > sense? Then the declaration + initializer would be just "AtomicFlag called;". > > FYI - Here is a link to the style guide: > "Do not define implicit conversions. Use the explicit keyword for conversion > operators and single-argument constructors." > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > tommi seems fine with this and tommi > style guide. hah! Yeah, well, maybe the class could be called AtomicBool to make it clearer that it should be considered the equivalent to the bool type. By having a default ctor though, do you consider that an implicit conversion to bool? (maybe I'm missing the point)
lgtm https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} On 2015/11/12 09:58:23, nisse wrote: > On 2015/11/12 09:20:51, magjed_webrtc wrote: > > On 2015/11/12 08:39:05, nisse wrote: > > > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > > > I'm not sure you are allowed to use implicit conversions like this. tommi > > > wdyt? > > > > > > I can make it explicit. Only reason for implicit was that I tried to > minimize > > > the changes where the flag is declared and initialized, from "bool called = > > > false;" to "AtomicFlag called = false;". With explicit, I have to change the > > > structure to "called(false)", which isn't a big deal, I guess. > > > > > > I'm also considering adding a default initial value of false, does that make > > > sense? Then the declaration + initializer would be just "AtomicFlag > called;". > > > > FYI - Here is a link to the style guide: > > "Do not define implicit conversions. Use the explicit keyword for conversion > > operators and single-argument constructors." > > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > > tommi seems fine with this and tommi > style guide. > > I changed the constructor to explicit (and with a default false). > Should I make the conversion to bool explicit too? Then I'd have to add some > conversion in the invocation of the EXPECT_TRUE_WAIT macro, which isn't too bad. Yes, you should probably make the conversion to bool explicit if you want to follow the style guide, and maybe also the copy assignment since you make a type conversion there. "Type conversion operators, and constructors that are callable with a single argument, must be marked explicit in the class definition. As an exception, copy and move constructors should not be explicit, since they do not perform type conversion. Implicit conversions can sometimes be necessary and appropriate for types that are designed to transparently wrap other types. In that case, contact your project leads to request a waiver of this rule." Tommi is the project lead, so it's up to him :) Default ctor is fine. Another FYI is that default arguments is only allowed in limited situations, but ctors is one of the exceptions: https://google.github.io/styleguide/cppguide.html#Default_Arguments
On 2015/11/12 10:16:31, tommi (webrtc) wrote: > https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.cc > File webrtc/base/thread_unittest.cc (right): > > https://codereview.webrtc.org/1439613004/diff/1/webrtc/base/thread_unittest.c... > webrtc/base/thread_unittest.cc:156: AtomicFlag(bool value) : flag(value) {} > On 2015/11/12 09:20:51, magjed_webrtc wrote: > > On 2015/11/12 08:39:05, nisse wrote: > > > On 2015/11/11 15:50:44, magjed_webrtc wrote: > > > > I'm not sure you are allowed to use implicit conversions like this. tommi > > > wdyt? > > > > > > I can make it explicit. Only reason for implicit was that I tried to > minimize > > > the changes where the flag is declared and initialized, from "bool called = > > > false;" to "AtomicFlag called = false;". With explicit, I have to change the > > > structure to "called(false)", which isn't a big deal, I guess. > > > > > > I'm also considering adding a default initial value of false, does that make > > > sense? Then the declaration + initializer would be just "AtomicFlag > called;". > > > > FYI - Here is a link to the style guide: > > "Do not define implicit conversions. Use the explicit keyword for conversion > > operators and single-argument constructors." > > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > > tommi seems fine with this and tommi > style guide. > > hah! Yeah, well, maybe the class could be called AtomicBool to make it clearer > that it should be considered the equivalent to the bool type. By having a > default ctor though, do you consider that an implicit conversion to bool? (maybe > I'm missing the point) The constructor is more like a conversion *to* bool.
lgtm with these nits addressed https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:154: class AtomicFlag { AtomicBool https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:162: operator bool() const { bool get_value() const {
https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:154: class AtomicFlag { On 2015/11/13 14:22:23, tommi (webrtc) wrote: > AtomicBool Done. https://codereview.webrtc.org/1439613004/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:162: operator bool() const { On 2015/11/13 14:22:24, tommi (webrtc) wrote: > bool get_value() const { Done. (But with the shorter name get()).
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1439613004/#ps60001 (title: "Addressed final(?) nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439613004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439613004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d9b75bef5d0749ea94b31cedab0105c241938954 Cr-Commit-Position: refs/heads/master@{#10645} |
