domfarolino / browser

Simple browser components
4 stars 1 forks source link

base::Thread::Quit() should join the thread. Possibly rename this API #41

Closed domfarolino closed 3 years ago

domfarolino commented 3 years ago

We should also consider whether or not we want base::Thread::Quit() to not only quit the delegate, but also join the underlying physical thread. Today, Quit() will get you out of Run(), which appears to eventually lead to Thread::ThreadFunc() being continued thus calling pthread_exit(0); on the underlying physical thread. But all of that stuff happens asynchronously. So if Start() is called quickly again on the base::Thread, there can technically be two physical threads running on the same base::Thread (the first one would be in Thread::ThreadFunc() cleaning up the place, and the second one would be in Thread::ThreadFunc() just starting to invoke ThreadMain() to get the delegate's Run() loop started. It shouldn't be possible for them to have observable effects on the other since they aren't touching the same data (one is just exiting the pthread, and no longer touches the underlying base::Thread object anymore), but this still seems awkward.

I think we should make base::Thread::Quit() join the underlying physical thread, so that Start() can only be called after the previous physical thread is completely exited .

Furthermore, in this codebase Quit() really refers to an asynchronous Quit() operation, and since this version of Quit() would synchronously wait for the underlying physical thread to join, we should rename the API to Stop(). We'll then have base::Thread APIs Start() and Stop(), as well as base::TaskLoop APIs Run() and Quit().