New issue
Advanced search Search tips

Issue 629716 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 622400



Sign in to add a comment

Add |joinable| to Thread::Options

Project Member Reported by gab@chromium.org, Jul 20 2016

Issue description

Required for MessageLoop backed non-joinable threads as discussed @
https://codereview.chromium.org/2103883004/#msg14

CL: https://codereview.chromium.org/2135413003/

Currently blocked on first cleaning up existing API with https://codereview.chromium.org/2145463002/

CC eventual reviewers FYI
 

Comment 1 by gab@chromium.org, Jul 20 2016

Blocking: 622400
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8504bffa5efb122e7e5ac960b745a82b79efef39

commit 8504bffa5efb122e7e5ac960b745a82b79efef39
Author: gab <gab@chromium.org>
Date: Thu Jul 21 13:42:57 2016

Remove unused base::Thread::thread_handle() getter.

Fix only user (dbus/test_server.cc) to use Thread on stack which is
automatically joined on destruction (and thus doesn't need the exposed
handle it was using to manually join).

BUG= 629716 

Review-Url: https://codereview.chromium.org/2162293002
Cr-Commit-Position: refs/heads/master@{#406831}

[modify] https://crrev.com/8504bffa5efb122e7e5ac960b745a82b79efef39/base/threading/thread.h
[modify] https://crrev.com/8504bffa5efb122e7e5ac960b745a82b79efef39/dbus/test_server.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1734a80f1519819eb6a3d8eb43064f8fb2c2e067

commit 1734a80f1519819eb6a3d8eb43064f8fb2c2e067
Author: gab <gab@chromium.org>
Date: Fri Jul 22 21:26:13 2016

Modernize base::Thread

No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001

In this CL:

 - Explicitly document the threading requirements of the API after grokking its use of locks
   in some places and lack of in some others)
   - (It's mostly thread unsafe; except for a few calls which are okay from the owner
      as well as the underlying thread; and one method which is actually thread-safe)
   - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139)
   - Remove |thread_lock_| which is unnecessary under the documented conditions
     (delayed until http://crbug.com/629139 is fixed..).

 - C++11 member initializers (makes it cleaner to add new POD members and
                              makes constructors simpler -- e.g. |run_loop_| was uninitialized)
 - Assertions and tests for current API which allows Start/Stop/Start cycles
   but wasn't tested... (actually more than that is poorly/not tested in this class
   but I'm merely adding tests for parts I'm stressing in my actual follow-up CL).

 - Comment nits

 - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool.

BUG= 629716 , 629139

Review-Url: https://codereview.chromium.org/2145463002
Cr-Commit-Position: refs/heads/master@{#407265}

[modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread.cc
[modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread.h
[modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread_unittest.cc

Comment 4 Deleted

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0cc00da87f2e1ccae783ad876848518b1c389f0

commit d0cc00da87f2e1ccae783ad876848518b1c389f0
Author: gab <gab@chromium.org>
Date: Mon Aug 01 19:24:03 2016

Loosen/improve AtomicFlag semantics.

In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.

But:
 1) What's required is *sequence* not *thread* checking.
 2) Calls to Set() need to be sequenced w.r.t. to each other, but not
    with respect to construction (AtomicFlag is not modified before Set()).

Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.

Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.

BUG= 629716 

Review-Url: https://codereview.chromium.org/2187333002
Cr-Commit-Position: refs/heads/master@{#409020}

[modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag.cc
[modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag.h
[modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b35b0572443f4a84377a1fbd199a1edabe3d7d6

commit 8b35b0572443f4a84377a1fbd199a1edabe3d7d6
Author: gab <gab@chromium.org>
Date: Thu Aug 04 17:59:56 2016

Add joinable option to SimpleThread::Options as was just done for Thread::Options.

SimpleThread will be more suitable than Thread for
https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop.

Also, cleanup the SimpleThread::Options API, making it more like Thread::Options.

BUG= 629716 
TBR=avi@chromium.org

Review-Url: https://codereview.chromium.org/2204333003
Cr-Commit-Position: refs/heads/master@{#409830}

[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.cc
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.h
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread_unittest.cc
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/content/renderer/categorized_worker_pool.cc

Comment 8 by gab@chromium.org, Aug 4 2016

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b35b0572443f4a84377a1fbd199a1edabe3d7d6

commit 8b35b0572443f4a84377a1fbd199a1edabe3d7d6
Author: gab <gab@chromium.org>
Date: Thu Aug 04 17:59:56 2016

Add joinable option to SimpleThread::Options as was just done for Thread::Options.

SimpleThread will be more suitable than Thread for
https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop.

Also, cleanup the SimpleThread::Options API, making it more like Thread::Options.

BUG= 629716 
TBR=avi@chromium.org

Review-Url: https://codereview.chromium.org/2204333003
Cr-Commit-Position: refs/heads/master@{#409830}

[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.cc
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.h
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread_unittest.cc
[modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/content/renderer/categorized_worker_pool.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459

commit 4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459
Author: reillyg <reillyg@chromium.org>
Date: Fri Aug 05 20:54:50 2016

Fix TSAN suppressions for non-joinable ThreadTest.

The compiler does not include the class name in the symbol name for the
TestBody method so the suppression added in r409915 doesn't work. This
patch disables the test under TSan instead.

BUG= 634383 , 629716 
TBR=thestig@chromium.org
NOTRY=true

Review-Url: https://codereview.chromium.org/2218663003
Cr-Commit-Position: refs/heads/master@{#410159}

[modify] https://crrev.com/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459/base/threading/thread_unittest.cc
[modify] https://crrev.com/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459/build/sanitizers/tsan_suppressions.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c

commit a9094bdb2cda679e8bf3a5b43b27c12efa975d3c
Author: justincohen <justincohen@chromium.org>
Date: Fri Aug 05 21:43:45 2016

Revert of Add joinable option to SimpleThread::Options as was just done for Thread.

(patchset #2 id:20001 of https://codereview.chromium.org/2204333003/ )

Reason for revert:
Reverting this since it is breaking the iOS base_unittests.

gba@ is working on a fix in https://codereview.chromium.org/2218983003/

Original issue's description:
> Add joinable option to SimpleThread::Options as was just done for Thread::Options.
>
> SimpleThread will be more suitable than Thread for
> https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop.
>
> Also, cleanup the SimpleThread::Options API, making it more like Thread::Options.
>
> BUG= 629716 
> TBR=avi@chromium.org
>
> Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6
> Cr-Commit-Position: refs/heads/master@{#409830}

TBR=thestig@chromium.org,avi@chromium.org,gab@chromium.org
BUG= 629716 

Review-Url: https://codereview.chromium.org/2219663006
Cr-Commit-Position: refs/heads/master@{#410169}

[modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread.cc
[modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread.h
[modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread_unittest.cc
[modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/content/renderer/categorized_worker_pool.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/addc8ef1bcae5bff546fba73eca8bea3e640ebcd

commit addc8ef1bcae5bff546fba73eca8bea3e640ebcd
Author: gab <gab@chromium.org>
Date: Sat Aug 13 17:28:50 2016

Add joinable option to SimpleThread::Options as was just done for Thread::Options.

SimpleThread will be more suitable than Thread for
https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop.

Also, cleanup the SimpleThread::Options API, making it more like Thread::Options.

Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses
member state (SimpleThread itself won't after invoking Run()).
DelegateSimpleThread provides a safe way to do this by explicitly supporting
being deleted while Run() is active but ensuring the Delegate itself outlives Run()).
Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is
eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this
is a good precursor to this paradigm (it really takes all its sense on base::Thread where
the destructor is more meaningful though -- winds down the MessageLoop).

The logic on SimpleThread is fairly simple, the complexity is in the assertions (which
were put behind DCHECK_IS_ON() to clearly isolate them) and the tests.

BUG= 629716 , 634835, 629139
TBR=avi@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng

Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6
Cr-Commit-Position: refs/heads/master@{#409830}

Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c
Review-Url: https://codereview.chromium.org/2204333003
Cr-Original-Commit-Position: refs/heads/master@{#410169}
Cr-Commit-Position: refs/heads/master@{#411897}

[modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread.cc
[modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread.h
[modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread_unittest.cc
[modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/content/renderer/categorized_worker_pool.cc

Sign in to add a comment