1211602 - Need to disable idle timeouts in nsThreadPool

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

I have a patch to try to get rid of some custom dynamic-thread-creation code with SharedThreadPool (which covers my usecase pretty well, at least for now!)

The problem is that io_thread_->IsOnCurrentThread(...) always fails.  Debugging into it, when calling from the correct thread, I get into nsThreadPool::IsOnCurrentThread(), and find that mThreads.Count() == 0 -- though we have a thread (mtransport #1) and are running on it.

I'll look deeper in a moment, but wanted to get it filed.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I *think* what might be happening is a callback on the thread occurs after it's been removed from the pool as idle - which I assumed wouldn't happen so long as I kept an open use, but I see there's a 60s timer to shut them down if idle

Comment 2

[Bobby Holley (:bholley)]

Why are you using IsOnCurrentThread on a SharedThreadPool? Shouldn't you be using a TaskQueue running on that ThreadPool?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I should be able to SetIdleThreadTimeout(UINT32_MAX), but that still can (in distant time) expire in 49 days, which is quite possible for some datachannel connections, etc.  We need a way to disable timeouts; either with no API surface change via SetIdleThreadTimeout(UINT32_MAX) or add a control to enable/disable idle timeouts on ThreadPools (which SharedThreadPool will inherit).

I'll file an additional bug about why I think the thread was still running anyways...  it appears that we are stuck in the ::Shutdown code on MainThread(), processing nested events.  Perhaps this is because ThreadPool assumes the parent thread is MainThread, while my pool is created and used from STS (Socket Thread), so the DispatchToMainThread of the SHutdown runnable might have issues, perhaps.  (IIRC Shutdown needs to be called from the parent/spawning thread, though I thought that was only a hard requirement on Windows).

And I'll check into using a TaskQueue instead... This seemed appropriate (and I'm sharing one thread from a number of pages/documents).

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Bobby Holley (:bholley) from comment #2)
> Why are you using IsOnCurrentThread on a SharedThreadPool? Shouldn't you be
> using a TaskQueue running on that ThreadPool?

I have upstream APIs (webrtc.org) which use same-thread guarantees to avoid needing locks.  I.e. if you shut down an channel, it checks that you're running on the same thread that started the channel and is used to control the channel.

I'd separately build LazySingleton thread class in media/mtransport to encapsulate a dynamic thread that starts when a user (or multiple users) appear, and stays around until the users are all done, and then will shut down.  I built this for mtransport, which uses this because the thread in question has to register with PBackground, so we have to stipulate the thread sticks around until we have no users. 

Instead of generalizing LazySingleton (or cloning it), I though it made more sense to re-use SharedThreadPool (and look at switching mtransport over as well).  In this case, we just need the same thread to keep the thread-sanity checks happy.  That maps pretty well onto SharedThreadPool with a single thread, except for the issue with being unable to remove the idle-shutdown aspect right now.

TaskQueue doesn't solve my problem - we actively don't want multiple threads for these uses, and TaskQueue abstracts a set of threads as if they were one pseudo-thread (more or less).

Comment 5

[Bobby Holley (:bholley)]

I see. The most ideal situation would be to generalize the upstream code such that the behavior of "am I on the right thread" is configurable via a callback, which would let you use a TaskQueue, where IsCurrentThreadIn() is supported and known to work. I understand if that's infeasible though.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Patch was posted and r+'d on bug 1198458 by froyd/jib

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcfdaee7edf

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bcfdaee7edf