Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26541005:
Avoid threading races on TraceSamplingThread's members. (Closed)
|
Created:
7 years, 2 months ago by haraken Modified:
7 years, 2 months ago CC:
chromium-reviews, dsinclair+watch_chromium.org, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAvoid threading races on TraceSamplingThread's members.
See a TSAN log in the bug for more details.
BUG=305459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228889
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Messages
Total messages: 19 (0 generated)
PTAL
Please remove the suppression and send a tryjob to linux_clang_tsan. Lgtm if ot passes. https://codereview.chromium.org/26541005/diff/1/base/debug/trace_event_impl.cc File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/1/base/debug/trace_event_impl.c... base/debug/trace_event_impl.cc:746: break; Nit: please fix the indentation
> Please remove the suppression and send a tryjob to linux_clang_tsan. I think I can get rid of three tests. Trying linux_clang_tsan... https://codereview.chromium.org/26541005/diff/1/base/debug/trace_event_impl.cc File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/1/base/debug/trace_event_impl.c... base/debug/trace_event_impl.cc:746: break; On 2013/10/09 06:55:32, Alexander Potapenko wrote: > Nit: please fix the indentation Done.
https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... base/debug/trace_event_impl.cc:767: void TraceSamplingThread::GetSamples() { Do these other methods need to be locked as well?
https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... base/debug/trace_event_impl.cc:767: void TraceSamplingThread::GetSamples() { On 2013/10/09 13:21:07, dsinclair wrote: > Do these other methods need to be locked as well? Yeah, the lock should also protect |sample_buckets_|. Let me update the CL tomorrow morning.
PTAL https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/3001/base/debug/trace_event_imp... base/debug/trace_event_impl.cc:767: void TraceSamplingThread::GetSamples() { On 2013/10/09 13:46:20, haraken wrote: > On 2013/10/09 13:21:07, dsinclair wrote: > > Do these other methods need to be locked as well? > > Yeah, the lock should also protect |sample_buckets_|. Let me update the CL > tomorrow morning. Fixed.
https://codereview.chromium.org/26541005/diff/11001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/11001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:768: Lock lock_; I think you want AutoLock lock(lock_) ? (and below)
https://codereview.chromium.org/26541005/diff/11001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/11001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:768: Lock lock_; On 2013/10/10 01:22:48, dsinclair wrote: > I think you want AutoLock lock(lock_) ? (and below) Sorry... fixed and verified compile.
lgtm
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
nduca: could you take a look as an OWNER?
nduca: ping? This is a pretty easy CL :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/26541005/19001
Message was sent while issue was closed.
Change committed as 228889
Message was sent while issue was closed.
hmm, this CL fixed the threading race, but caused a dead lock in HeapChecker tests. https://code.google.com/p/chromium/issues/detail?id=308273 Looks mysterious. There would be no path that causes a dead lock... https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:771: bucket_data->callback.Run(bucket_data); The only suspicious place is this one. We're calling a callback while holding a lock. However, this wouldn't be a problem because the callback (DefautSamplingCallback) doesn't acquire the lock...
Message was sent while issue was closed.
+wangxianzhu Nothing is jumping out at me, adding wangxianzhu as he did a lot of thread work in the tracing code, maybe something will jump out at him.
Message was sent while issue was closed.
https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:727: cancellation_flag_.reset(new CancellationFlag); It looks weird that CancellationFlag and WaitableEvent are used along with a lock. Both of the are thread-safe. I think you can avoid the lock by using CancellationFlag directly not through a scoped_ptr. I think WaitableEvent is the same if the tests allocate it on stack and this class just holds a non-owning pointer: https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:771: bucket_data->callback.Run(bucket_data); On 2013/10/17 01:15:51, haraken wrote: > > The only suspicious place is this one. We're calling a callback while holding a > lock. However, this wouldn't be a problem because the callback > (DefautSamplingCallback) doesn't acquire the lock... Please add comments somewhere to state the above restriction. https://codereview.chromium.org/26541005/diff/19001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:771: bucket_data->callback.Run(bucket_data); On 2013/10/17 01:15:51, haraken wrote: > > The only suspicious place is this one. We're calling a callback while holding a > lock. However, this wouldn't be a problem because the callback > (DefautSamplingCallback) doesn't acquire the lock...
Message was sent while issue was closed.
Sorry just noticed the dead lock issue mentioned in #15. haraken@ can you provide a link to the failure?
Message was sent while issue was closed.
On 2013/10/17 19:18:34, Xianzhu wrote: > Sorry just noticed the dead lock issue mentioned in #15. > > haraken@ can you provide a link to the failure? Thanks, Xianzhu! This one: https://code.google.com/p/chromium/issues/detail?id=308273 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
