Move thread from test/ to util/thread/.

Move thread from test/ to util/thread/.

BUG=
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/f357afc43e20b1e405dc06a59184f43feb71d9d0

[erikwright (departed), 2015-05-11 15:48:40]

erikwright@chromium.org changed reviewers:
+ mark@chromium.org

[erikwright (departed), 2015-05-11 15:48:41]

Mark, PTAL.

There could be some discussion about how to do error handling in production
code. I used PCHECK for now, as a failure to start (or join) a thread could lead
to pretty bad results. Another option could be to return an error to the client,
but this is presumably pretty rare and occurs when the system is already
unstable and our chances of doing something useful afterwards seem low.

[Sigurður Ásgeirsson, 2015-05-11 15:54:03]

siggi@chromium.org changed reviewers:
+ siggi@chromium.org

[Sigurður Ásgeirsson, 2015-05-11 15:54:04]

Hey guys,

why isn't this using base::Thread?

Siggi

[Sigurður Ásgeirsson, 2015-05-11 15:54:05]

Hey guys,

why isn't this using base::Thread?

Siggi

[scottmg, 2015-05-11 21:43:32]

On 2015/05/11 15:54:05, Sigurður Ásgeirsson wrote:
> Hey guys,
> 
> why isn't this using base::Thread?
> 
> Siggi

I don't think there's any major reason. The implementation of base::Thread is
fairly large, and is not currently in mini_chromium. It would involve pulling in
things like MessageLoop, which pulls in the TaskRunners, which pulls in Bind,
all of which would be unnecessary weight in Crashpad. (Mark will probably have a
more reasoned answer, but he's out on leave now.)

[scottmg, 2015-05-11 21:51:59]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-05-11 21:52:00]

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc
File util/thread/thread_test.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:78: Semaphore unblock_wait_thread(0);
`unblock_wait_thread` makes it sound like a thread on line 80. Can you call this
`unblock_wait_thread_semaphore` or `unblock_waiter`?

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:85: ASSERT_FALSE(join_completed.TimedWait(.1));
I don't like this timeout too much. I think just waiting and having the test
block indefinitely would be preferable to potential flakiness on a slow bot.

[Robert Sesek, 2015-05-11 21:57:01]

On 2015/05/11 21:43:32, scottmg wrote:
> On 2015/05/11 15:54:05, Sigurður Ásgeirsson wrote:
> > Hey guys,
> > 
> > why isn't this using base::Thread?
> > 
> > Siggi
> 
> I don't think there's any major reason. The implementation of base::Thread is
> fairly large, and is not currently in mini_chromium. It would involve pulling
in
> things like MessageLoop, which pulls in the TaskRunners, which pulls in Bind,
> all of which would be unnecessary weight in Crashpad. (Mark will probably have
a
> more reasoned answer, but he's out on leave now.)

This is basically correct; the base implementation of Thread is quite
complicated. mini_chromium and Crashpad do not have a need for MessageLoop (we
have discussed having a potential event loop primitive, but it so far has not
been necessary), and we certainly do not want to pull in all of Bind.

If anything, mini_chromium could pull in base/threading/simple_thread.h, which
is a much smaller API surface and very close to the abstraction being created
here. That's still not great, though, since it would require pulling
waitable_event.h, which also pulls in ref_counted.h on POSIX.

[Robert Sesek, 2015-05-11 22:10:51]

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc
File util/thread/thread_posix.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc...
util/thread/thread_posix.cc:24: PCHECK(0 == rv);
Use PCHECK_EQ here and on line 30.

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc
File util/thread/thread_test.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:85: ASSERT_FALSE(join_completed.TimedWait(.1));
On 2015/05/11 21:52:00, scottmg wrote:
> I don't like this timeout too much. I think just waiting and having the test
> block indefinitely would be preferable to potential flakiness on a slow bot.

+1. This will be flaky.

[erikwright (departed), 2015-05-13 15:27:50]

PTAL.

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc
File util/thread/thread_posix.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc...
util/thread/thread_posix.cc:24: PCHECK(0 == rv);
On 2015/05/11 22:10:51, Robert Sesek wrote:
> Use PCHECK_EQ here and on line 30.

Done.

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc
File util/thread/thread_test.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:78: Semaphore unblock_wait_thread(0);
On 2015/05/11 21:52:00, scottmg wrote:
> `unblock_wait_thread` makes it sound like a thread on line 80. Can you call
this
> `unblock_wait_thread_semaphore` or `unblock_waiter`?

Done.

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:85: ASSERT_FALSE(join_completed.TimedWait(.1));
On 2015/05/11 21:52:00, scottmg wrote:
> I don't like this timeout too much. I think just waiting and having the test
> block indefinitely would be preferable to potential flakiness on a slow bot.

I think you misunderstood the purpose of this timeout. We don't expect the wait
to ever complete. But if the implementation were broken, it might. So we wait a
little bit of time to ensure that it is, in fact, blocking. While this could
possibly give false positives (if it takes more than 100ms for
JoinAndSignalThread::ThreadMain to start) it should never give a false negative.

If I remove this test I don't know of any other way to assert that Join()
actually blocks.

[scottmg, 2015-05-13 17:31:49]

lgtm

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc
File util/thread/thread_test.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_test.cc#...
util/thread/thread_test.cc:85: ASSERT_FALSE(join_completed.TimedWait(.1));
On 2015/05/13 15:27:49, erikwright wrote:
> On 2015/05/11 21:52:00, scottmg wrote:
> > I don't like this timeout too much. I think just waiting and having the test
> > block indefinitely would be preferable to potential flakiness on a slow bot.
> 
> I think you misunderstood the purpose of this timeout. We don't expect the
wait
> to ever complete. But if the implementation were broken, it might. So we wait
a
> little bit of time to ensure that it is, in fact, blocking. While this could
> possibly give false positives (if it takes more than 100ms for
> JoinAndSignalThread::ThreadMain to start) it should never give a false
negative.
> 
> If I remove this test I don't know of any other way to assert that Join()
> actually blocks.

I guess we would need a WFMO version to avoid the timeout. Thanks for adding the
comment.

[erikwright (departed), 2015-05-13 18:02:55]

Patchset #3 (id:40001) has been deleted

[erikwright (departed), 2015-05-13 18:05:46]

FYI - small fix for posix.

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc
File util/thread/thread_posix.cc (right):

https://codereview.chromium.org/1134943003/diff/1/util/thread/thread_posix.cc...
util/thread/thread_posix.cc:24: PCHECK(0 == rv);
On 2015/05/13 15:27:49, erikwright wrote:
> On 2015/05/11 22:10:51, Robert Sesek wrote:
> > Use PCHECK_EQ here and on line 30.
> 
> Done.

Undone. There is no PCHECK_EQ.

[erikwright (departed), 2015-05-13 18:06:02]

Committed patchset #3 (id:60001) manually as
f357afc43e20b1e405dc06a59184f43feb71d9d0 (presubmit successful).