Thread fixes for shutdown.

Thread fixes for shutdown.

- Don't disable thread creation until isolates have shutdown.
- A thread-pool thread may try to start up after thread creation has
  been disabled. We must reap the Worker objects for these threads.
- A thread may try to start up after OSThread::thread_list_lock_ has
  been destroyed. Detect this.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/a6818de006a5bbd364a392ef44fa1c256e2da9da

[zra, 2015-12-30 16:40:08]

zra@google.com changed reviewers:
+ asiva@google.com

[siva, 2015-12-30 20:51:02]

LGTM with one comment.

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:120: current->Shutdown();
Why is it necessary to call current->Shutdown() when (currentId == id)?

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:268: void ThreadPool::AddWorkerToShutdownList(Worker*
worker) {
ASSERT that the exit_monitor lock is held by the current thread?

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:277: ASSERT(shutting_down_workers_ != NULL);
ASSERT that the exit_monitor lock is held by the current thread?

[zra, 2015-12-30 22:36:28]

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:120: current->Shutdown();
On 2015/12/30 20:51:02, siva wrote:
> Why is it necessary to call current->Shutdown() when (currentId == id)?

Thinking about this more, it is not safe for a thread-pool thread to call
ThreadPool::Shutdown(), so I've added an assert for that.

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:268: void ThreadPool::AddWorkerToShutdownList(Worker*
worker) {
On 2015/12/30 20:51:02, siva wrote:
> ASSERT that the exit_monitor lock is held by the current thread?

Done.

https://codereview.chromium.org/1555643002/diff/20001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:277: ASSERT(shutting_down_workers_ != NULL);
On 2015/12/30 20:51:02, siva wrote:
> ASSERT that the exit_monitor lock is held by the current thread?

Done.

[zra, 2015-12-30 23:01:35]

Description was changed from

==========
Thread fixes for shutdown.

- Don't disable thread creation until isolates have shutdown.
- A thread-pool thread may try to start up after thread creation has
  been disabled. We must reap the Worker objects for these threads.
- A thread may try to start up after OSThread::thread_list_lock_ has
  been destroyed. Detect this.
==========

to

==========
Thread fixes for shutdown.

- Don't disable thread creation until isolates have shutdown.
- A thread-pool thread may try to start up after thread creation has
  been disabled. We must reap the Worker objects for these threads.
- A thread may try to start up after OSThread::thread_list_lock_ has
  been destroyed. Detect this.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a6818de006a5bbd364a392ef44fa1c256e2da9da
==========

[zra, 2015-12-30 23:01:36]

Committed patchset #3 (id:40001) manually as
a6818de006a5bbd364a392ef44fa1c256e2da9da (presubmit successful).

[Vyacheslav Egorov (Google), 2016-01-02 16:41:29]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2016-01-02 16:41:30]

driveby

https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc...
runtime/vm/os_thread.cc:43: MutexLocker ml(thread_list_lock_);
Code like this is a red-flag in multithreaded environment for two reasons:

1. You fetch a value twice and expect that it does not change in between but
there is no lock spanning these two accesses. Depending on how compiler compiles
this code you might end up with a race.

2. Manual managed lifetime of the mutex is bad. Mutex might be freed by another
thread right when we try to acquire it, so even rewriting the code like this:

Mutex* lock = thread_list_lock_;
if (lock == NULL) return NULL;
MutexLocker ml(lock);

guarantees *nothing*. 

Global locks should usually have static lifetime to guarantee correctness.

[siva, 2016-01-03 03:26:49]

On 2016/01/02 16:41:30, Vyacheslav Egorov (Google) wrote:
> driveby
> 
> https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc
> File runtime/vm/os_thread.cc (right):
> 
>
https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc...
> runtime/vm/os_thread.cc:43: MutexLocker ml(thread_list_lock_);
> Code like this is a red-flag in multithreaded environment for two reasons:
> 
> 1. You fetch a value twice and expect that it does not change in between but
> there is no lock spanning these two accesses. Depending on how compiler
compiles
> this code you might end up with a race.
> 
> 2. Manual managed lifetime of the mutex is bad. Mutex might be freed by
another
> thread right when we try to acquire it, so even rewriting the code like this:
> 
> Mutex* lock = thread_list_lock_;
> if (lock == NULL) return NULL;
> MutexLocker ml(lock);
> 
> guarantees *nothing*. 
> 
> Global locks should usually have static lifetime to guarantee correctness.

Not sure making the lock static solves the problem as C++ static destructors run
in random order causing problems. I guess we should not delete the lock and
thread local entry at all. The caveat is we leak this piece of memory, how many
times is an embedder going to call Dart_Cleanup and then restart the VM using
Dart_InitOnce to make this leak a significant one. I vote for not deleting this
lock and the associated thread local entry.

[Florian Schneider, 2016-01-04 08:38:42]

On 2016/01/03 03:26:49, siva wrote:
> On 2016/01/02 16:41:30, Vyacheslav Egorov (Google) wrote:
> > driveby
> > 
> >
https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc
> > File runtime/vm/os_thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc...
> > runtime/vm/os_thread.cc:43: MutexLocker ml(thread_list_lock_);
> > Code like this is a red-flag in multithreaded environment for two reasons:
> > 
> > 1. You fetch a value twice and expect that it does not change in between but
> > there is no lock spanning these two accesses. Depending on how compiler
> compiles
> > this code you might end up with a race.
> > 
> > 2. Manual managed lifetime of the mutex is bad. Mutex might be freed by
> another
> > thread right when we try to acquire it, so even rewriting the code like
this:
> > 
> > Mutex* lock = thread_list_lock_;
> > if (lock == NULL) return NULL;
> > MutexLocker ml(lock);
> > 
> > guarantees *nothing*. 
> > 
> > Global locks should usually have static lifetime to guarantee correctness.
> 
> Not sure making the lock static solves the problem as C++ static destructors
run
> in random order causing problems. I guess we should not delete the lock and
> thread local entry at all. The caveat is we leak this piece of memory, how
many
> times is an embedder going to call Dart_Cleanup and then restart the VM using
> Dart_InitOnce to make this leak a significant one. I vote for not deleting
this
> lock and the associated thread local entry.

Destructors are called when exit() is called or the main function returns. If we
guarantee that we do exit() only from the main thread, this should be fixed,
shouldn't it?

I don't think we should leak something from inside the VM (i.e. runtime/vm) -
runtime/bin - the standalone embedder is a different story: I already "fixed"
races by not deleting stuff with static lifetime.

[zra, 2016-01-04 17:19:10]

I think Siva is right and this lock will be difficult to delete. I'll remove the
deletion, and add some comments explaining the situation.

https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1555643002/diff/40001/runtime/vm/os_thread.cc...
runtime/vm/os_thread.cc:43: MutexLocker ml(thread_list_lock_);
On 2016/01/02 16:41:30, Vyacheslav Egorov (Google) wrote:
> Code like this is a red-flag in multithreaded environment for two reasons:
> 
> 1. You fetch a value twice and expect that it does not change in between but
> there is no lock spanning these two accesses. Depending on how compiler
compiles
> this code you might end up with a race.
> 
> 2. Manual managed lifetime of the mutex is bad. Mutex might be freed by
another
> thread right when we try to acquire it, so even rewriting the code like this:
> 
> Mutex* lock = thread_list_lock_;
> if (lock == NULL) return NULL;
> MutexLocker ml(lock);
> 
> guarantees *nothing*. 
> 
> Global locks should usually have static lifetime to guarantee correctness. 

Acknowledged.