Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736763006:
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. (Closed)
DescriptionFix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
Committed: https://crrev.com/292d658b2035a5ec6c7611bf2cd85e33168f07c7
Cr-Commit-Position: refs/heads/master@{#12027}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rewrote the result handling using locking instead of std::atomic<>. #Patch Set 3 : Removed suppressions for SendSrtpToSrtpOnThread #
Total comments: 4
Patch Set 4 : Moved two stars. #Messages
Total messages: 35 (16 generated)
Description was changed from ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. BUG=5524 ========== to ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. BUG=5524 ==========
ossu@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Made a small fix for the data race in SendRtpToRtpOnThread and SendSrtpToSrtpOnThread in channel_unittest.cc. It intermittently triggers tsan2. I've validated the fix locally by making the CallThread code much worse (writing 100,000 times to the pointer) and running tsan2 on it. Without the fix it warns every time - with it tsan2 runs fine. https://codereview.webrtc.org/1736763006/diff/1/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/1736763006/diff/1/webrtc/pc/channel_unittest.cc... webrtc/pc/channel_unittest.cc:420: *result_ = (*obj_.*method_)(); Since the pointer is always written to in the constructor, there's no reason to check if it's valid here.
Description was changed from ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. BUG=5524 ========== to ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. I'm aware std::atomic<> is currently forbidden, due to performance reasons. I believe this needn't apply to test-specific code but if it does, I'll roll another version that works around this, using locking instead. BUG=5524 ==========
lgtm, as long as there's not some second reason to avoid <atomic> (besides performance) that's not mentioned. https://codereview.webrtc.org/1736763006/diff/1/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/1736763006/diff/1/webrtc/pc/channel_unittest.cc... webrtc/pc/channel_unittest.cc:420: *result_ = (*obj_.*method_)(); On 2016/03/01 15:00:38, ossu wrote: > Since the pointer is always written to in the constructor, there's no reason to > check if it's valid here. I think the purpose of the check was so that you can create a CallThread with a null result parameter, in case you don't care about the result. But it appears that doesn't happen anywhere.
pthatcher@webrtc.org changed reviewers: + tommi@webrtc.org
+tommi for his opinion on std::atomic<> I'm fine with this CL if he is fine with the use of std::atomic<> in tests.
tommi@chromium.org changed reviewers: + tommi@chromium.org
std::atomic is still on the explicitly-banned list in Chromium, so let's wait for the feature to be discussed there since we could be creating problems for us by starting to use it. In the meantime, we do have atomicops.h in base that we can use.
On 2016/03/03 11:44:28, tommi-chromium wrote: > std::atomic is still on the explicitly-banned list in Chromium, so let's wait > for the feature to be discussed there since we could be creating problems for us > by starting to use it. In the meantime, we do have atomicops.h in base that we > can use. see here: https://chromium-cpp.appspot.com/
On 2016/03/03 11:44:43, tommi-chromium wrote: > On 2016/03/03 11:44:28, tommi-chromium wrote: > > std::atomic is still on the explicitly-banned list in Chromium, so let's wait > > for the feature to be discussed there since we could be creating problems for > us > > by starting to use it. In the meantime, we do have atomicops.h in base that > we > > can use. > > see here: https://chromium-cpp.appspot.com/ Fair enough. I'll take a look at a version using atomicops.h instead.
Description was changed from ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. I'm aware std::atomic<> is currently forbidden, due to performance reasons. I believe this needn't apply to test-specific code but if it does, I'll roll another version that works around this, using locking instead. BUG=5524 ========== to ========== Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread. Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc. I'm aware std::atomic<> is currently forbidden, due to performance reasons. I believe this needn't apply to test-specific code but if it does, I'll roll another version that works around this, using locking instead. BUG=webrtc:5524 ==========
Description was changed from
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed bool to std::atomic<bool> in CallThread et al in channel_unittest.cc.
I'm aware std::atomic<> is currently forbidden, due to performance reasons. I
believe this needn't apply to test-specific code but if it does, I'll roll
another version that works around this, using locking instead.
BUG=webrtc:5524
==========
to
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
==========
Looked at atomicops but found it more straight-forward to use locking, since either solution would require a bit of a rewrite.
lgtm; thanks a bunch for taking the time to address flaky tests. Also, you might as well also remove the suppression in webrtc/build/sanitizers/tsan_suppressions_webrtc.cc in this CL.
No problem! I had a little time to spend and I just know how annoying it is when you have tests that you can't rely on; even if it only happens once in a while, someone new's going to have to learn that a particular error isn't really an error and it slows down the whole process. Or worse, you start ignoring proper errors because "these tests can be a bit flaky".
Alright. Pushed this through trybots and they seem happy. Could someone with owner status please take another look?
lgtm I'd prefer you simplify it into one class, but I won't block being able to submit to fix the tests. https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... webrtc/pc/channel_unittest.cc:412: CallThread(ChannelTest<T>* obj, Method method, bool *result = nullptr) bool* result https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... webrtc/pc/channel_unittest.cc:1672: CallOnThreadAndWaitForDone(&ChannelTest<T>::SendRtcp1, &send_rtcp1); Could you convert this call to using the new style also? Then CallThread could just be ScopedCallThread and be more simple.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... webrtc/pc/channel_unittest.cc:412: CallThread(ChannelTest<T>* obj, Method method, bool *result = nullptr) On 2016/03/15 16:38:20, pthatcher1 wrote: > bool* result Acknowledged. https://codereview.webrtc.org/1736763006/diff/40001/webrtc/pc/channel_unittes... webrtc/pc/channel_unittest.cc:1672: CallOnThreadAndWaitForDone(&ChannelTest<T>::SendRtcp1, &send_rtcp1); On 2016/03/15 16:38:20, pthatcher1 wrote: > Could you convert this call to using the new style also? Then CallThread could > just be ScopedCallThread and be more simple. I had hoped to do just that, but if I understand correctly, CallThread is an rtc::SignalThread, and will thus get deleted automatically once it's been Release()d or Destroy()ed (and the thread has finished). Since I can't put it on the stack, I had to wrap it to ensure it was cleaned up before exiting the scope. It might be possible to do with other primitives, like using rtc::Thread directly, but I don't feel up-to-speed enough on threading in WebRTC to do that change - there's probably a reason SignalThread was chosen as the base class of CallThread.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1736763006/#ps100001 (title: "Moved two stars.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736763006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736763006/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4177)
ossu@webrtc.org changed reviewers: + kjellander@webrtc.org
Added kjellander as reviewer, since we need an owner of the suppressions to give their thumbs up as well.
On 2016/03/16 14:05:50, ossu wrote: > Added kjellander as reviewer, since we need an owner of the suppressions to give > their thumbs up as well. webrtc/build/sanitizers/tsan_suppressions_webrtc.cc: lgtm
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736763006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736763006/100001
Message was sent while issue was closed.
Description was changed from
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
==========
to
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
==========
to
==========
Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and
SendSrtpToSrtpOnThread.
Changed the channel unittest to use locking when reading/writing the
result variable. To do this, I had to move the result into the thread
object, which in turn required me to properly handle the lifetime of the
thread object, since it cannot disappear while we want to read the
result.
It is still possible to have the result being written to a local
variable, but it will only be updated as the thread object is
destroyed. It is used to for the implementation of
CallOnThreadAndWaitForDone. The old CallOnThread is gone and replaced by
ScopedCallThread instead.
BUG=webrtc:5524
Committed: https://crrev.com/292d658b2035a5ec6c7611bf2cd85e33168f07c7
Cr-Commit-Position: refs/heads/master@{#12027}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/292d658b2035a5ec6c7611bf2cd85e33168f07c7 Cr-Commit-Position: refs/heads/master@{#12027} |
