658460 - XPCOM Shutdown can't explicitly shut down ThreadPool threads

Status: UNCONFIRMED

(Core :: XUL, defect)

Description

[Jan Larres [:jlarres]]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: 

ThreadPool threads that are in waiting in nsThreadPool::Run() are waiting on the ThreadPool monitor. ShutdownXPCOM tries to explicitly shut down all threads through the ThreadManager, but the shutdown is posted to the thread directly and thus bypasses the ThreadPool Monitor, thus failing to wake up the responsible thread to handle the event. So if for example the ThreadPool timeout is very high Firefox will hang on shutdown until the timeout expires.

Reproducible: Always

Comment 1

[Jan Larres [:jlarres]]

I am currently experimenting with making the ThreadPool more deterministic by setting low thread limits and high timeouts so the number of thread creation/destruction events get reduced. However, on shutdown the ThreadPool that is used by the nsParser component is only explicitly shut down after the ThreadManager. This causes the main thread to continuously generate dummy events while it is waiting for the ThreadPool threads to shut down.

I'll try to make the ThreadPool explicitly observe the XPCOM shutdown event so it can shut down all of its threads directly.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

All threads are supposed to be joined during the "shutdown-threads" observer notification, and the thread pool must be shut down manually. See, for example:

https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IndexedDatabaseManager.cpp#265

Comment 3

[Jan Larres [:jlarres]]

The "shutdown-threads" notification seems to only be observed in a few places, and notably not in the thread pools (or the threads themselves). I don't know if that's intentional (as your comment seems to suggest), but it leads to this case where threads can't be shut down properly. I've written a patch that makes the thread pools observe the "shutdown-threads" notification, and it solves the problem for me. It does have the slight side effect that the Shutdown() method gets called twice for thread pools that still exist after the notification, but since the only thing that method does is shut down all the pool's threads it shouldn't do any harm.

Note that if the HTML5 parser is enabled the problem currently doesn't show itself since that parser doesn't use a thread pool, but if any other component starts using one it would appear again.

Comment 4

[Jan Larres [:jlarres]]

Attachment: Make nsThreadPool observe xpcom-shutdown-threads notification

Comment 5

[Benjamin Smedberg]

Comment on attachment 535903 [details] [diff] [review]
Make nsThreadPool observe xpcom-shutdown-threads notification

You never clear the observer anywhere. Assuming there are any thread pools which may not live as long as the app, you'd be holding those alive until shutdown.

Is there a reason why we want threadpools to automagically shut themselves down, instead of requiring (asserting) that clients which create thread pools shut them down properly, just as clients who create individual threads are suppose to do?

Comment 6

[Jan Larres [:jlarres]]

The reason is that ShutdownXPCOM tries to shut down all remaining threads (here: http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#625 ) before the components (that could use a threadpool, like with the old html parser) (here: http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#694 ). But the threadmanager can't notify threadpool threads of the shutdown event since they are blocked on the threadpool monitor, which is out of the threadmanager's reach. So the shutdown event will only get processed once the thread's timeout is reached, which could potentially be a while. Therefore I think that shutting down the threads during the normal shutdown-threads notification makes sense since then the the threadmanager doesn't have to wait for those threads (since the notification happens earlier).

As for clearing the observer, I guess the only reasonable place to put it would be the nsThreadPool::Shutdown() method, even though it is called from the destructor. Would calling RemoveObserver() twice be a problem?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> As for clearing the observer, I guess the only reasonable place to put it
> would be the nsThreadPool::Shutdown() method, even though it is called from
> the destructor. Would calling RemoveObserver() twice be a problem?

I don't think so, but you can store a flag to indicate whether you've called it or not. Or you could just remomve it from the destructor and require Shutdown to be called.

Comment 8

[Jan Larres [:jlarres]]

Attachment: Updated patch that removes the observer on shutdown

OK, I'm now checking the mShutdown member which gets set during (threadpool) shutdown anyway so the observer will only be removed once. I've left the call from the destructor in since it doesn't seem to have caused any problems so far, and the observer is now guarded.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

You still haven't addressed comment 5:

> Is there a reason why we want threadpools to automagically shut themselves
> down, instead of requiring (asserting) that clients which create thread
> pools shut them down properly, just as clients who create individual threads
> are suppose to do?

Comment 10

[Jan Larres [:jlarres]]

(In reply to comment #9)

I thought I already explained it in comment 6. Threads in threadpools can only be shut down from inside the pool, so ShutdownXPCOM can't do that explicitly from the outside using the thread manager in http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#625 , meaning that this call will hang until all threadpool threads that still exist at this point have timed out. This can happen if a component uses a threadpool with a long thread timeout, since components are only shut down *after* that call, so they can't shut down the threadpools before it. Or did you mean something else?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #10)

The question is *why* do we want this to happen automatically. You've explained *how* it can happen, but the situation you describe would be a bug in my view. Users of the threadpool can and should take responsibility to clean up after themselves in my opinion.

I can think of at least one scenario where having a threadpool stop accepting events at some random time could cause real problems. Imagine some threadpool that is being used for a map/reduce operation, where the final reduce step is to be run on one of the pool's threads. That reduce can't begin until all the map threads have returned results, but if we hit ShutdownXPCOM before the final map has completed then the reduce step will not ever happen. If the reduce was supposed to write data to the disk or something then we'd be talking about data loss.

I don't think that the benefits of your patch (faster shutdown if some developer uses threadpools incorrectly) outweigh the hazards (threads magically dying) here.

Comment 12

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336