Declaration and deployment of threads and thread pools in th

[dmlloyd]

I've developed an experimental POJO-based deployer for defining thread groups, thread factories, and thread pools for the MC. The source is in my sandbox area at https://svn.jboss.org/repos/sandbox/david.lloyd/jboss-threads/trunk.

Basically it lets you write a jboss-threads.xml file which contains declarations like thread-group, thread-factory, thread-pool-executor, etc. You can configure things like the daemon status, priority, set parent groups, pool sizes (scaled linearly or by CPU count, or both), the queue type and rejection policies for the executor, etc. These get created as named beans which can be injected using regular POJO deployment stuff.

Since it's just an experiment I didn't go through the mbean stuff to see if every little function is replicated but this should be enough to tell whether it's a good idea or a dumb idea anyway. At the very least it should make thread pool configuration a lot more uniform, and hopefully simpler for the end-user...

I don't have a build script yet, but you can use your IDE of choice to compile the two modules into a single JAR and deploy it as a deployer. The XSD for the jboss-threads.xml files is in there too.

[alesj]

Nice!

But I guess you should provide some examples.
e.g. few xml snippets, explaining what does what

ps: I'll definitely mention this in my MC's VDF article ;-)

[adrian.brock]

Looks good (but I only gave it a quick scan).
Obviously it needs some tests to avoid regressions. :-)

A few issues:

* THREAD PRIORITY

Why are you using float for thread priority?
A thread can only take an integer priority between
1 (Thread.MIN_PRIORiTY) and 10 (Thread.MAX_PRIORITY)

Even if it was a decimal, BigDecimal would be better than
float because of the rounding errors.
See for example the xsd:decimal bindings here:
http://www.xyzws.com/scdjws/studyguide/jaxb_samples2.0.html
or look at the jaxb spec for recommended bindings.

* MBEANS

You can inject into MBeans just like you can pojos

* JBoss's BasicThreadPool and Task abstraction

We already have an abstraction in the common-core project
that adds to the basic thread pool executor abstraction.
(Mostly by wrapping it).

This adds things like lifecycle and error handling callbacks via the task abstraction,
and management exposure via an MBean.

But there are also some different policies such as MinThreadPoolExecutor
so you can avoid the known race condition if you implement a WAIT policy
when the thread pool is full (something that is not part of java.util.concurrent
because of the race condition)

[dmlloyd]

Why are you using float for thread priority?
A thread can only take an integer priority between
1 (Thread.MIN_PRIORiTY) and 10 (Thread.MAX_PRIORITY)

I got the notion in my head that MAX_PRIORITY might not be 10, so my thought was to take a float between 1.0 and 10.0 inclusive and scale it between MIN and MAX. Though I guess I overthought it, since if someone decided to arbitrarily change the priority scale, there would probably be compatibility issues, so that would seem to be a rather unlikely scenario.

Even if it was a decimal, BigDecimal would be better than float because of the rounding errors.

I doubt that someone would care if their thread at priority 10.1 was really running at 10.099999999999999644728632119950, since it would very likely end up being the same priority value anyway :-)

In any case, making this a float was probably a dumb idea - I think I'll just make it an int.

But there are also some different policies such as MinThreadPoolExecutor so you can avoid the known race condition if you implement a WAIT policy when the thread pool is full (something that is not part of java.util.concurrent because of the race condition)

I saw the code and the comment to that effect, but at first glance it wasn't clear what the race condition was, or how the fix worked. I'll have to look at that one more time...

[adrian.brock]

But there are also some different policies such as MinThreadPoolExecutor so you can avoid the known race condition if you implement a WAIT policy when the thread pool is full (something that is not part of java.util.concurrent because of the race condition)

I saw the code and the comment to that effect, but at first glance it wasn't clear what the race condition was, or how the fix worked. I'll have to look at that one more time...

The issue comes with small thread pools. e.g. a singleton MDB configuration
has a thread pool size of 1

Depending upon the reaping policy of unused threads it is possible for a race
condition to go something like this (I'll assume no queue although it makes
no real difference in practice since the race would still happen after dequeuing):

1) Thread 1: Submit work to the pool
2) Thread 2: Execute the work
3) Thread 2: Complete work
4) Thread 1: Submit some work - pool is busy so wait
5) Thread 2: Return thread to the pool
6) // CPU starvation for longer the idle time of threads
7) Thread 1: Stop waiting and check we have enough threads (we do)
8) Thread 2: I've been idle too long so end

So you end up with thread 1 thinking there are enough threads
to execute the work, but Thread 2 has stopped an instant after the check.

The work will never be executed unless some other thread also tries
to execute some work on the pool (meaning the number of threads gets rechecked).
For a singleton MDB with a read-ahead of messages of size 1 this never occurs
so message delivery to the MDB stalls.

The fix/work around we have is to stop step (8) from occuring.
i.e. you never allow the thread pool to go below a minimum of one thread
which avoids the race condition - there is always at least one thread
available to execute the work.

The real fix would be to make the check in step (7) more atomic
i.e. move the thread pool size check to later when it tries to execute the work
but that's not easy to do if you look at the code. :-)

[adrian.brock]

4) Thread 1: Submit some work - pool is busy so wait

NOTE: Without this requirement in step 4 which isn't a policy available in java.util.concurrent
you can't have the race condition. The submission of the work will either
throw an error immediately, after a timeout or run the work on Thread 1

But for an MDB singleton none of those options are what we want. ;-)

[dmlloyd]

"alesj" wrote:
Nice!

But I guess you should provide some examples.
e.g. few xml snippets, explaining what does what

OK, it's pretty straightforward. Basically if you have a pojo component that requires an executor, you can build it declaratively via XML (imagine a security manager scenario - by building your executor this way, your code needs no special thread-control permissions).

So to create a thread pool for my application which keeps from 2 + 1.2n to 4 + 2n threads (where n = # of CPUs), with a bounded queue and a blocking reject-policy, I would just add a jboss-threads.xml file like this:

<threads xmlns="urn:jboss:threads:1.0">
 <thread-group name="MyThreadGroup" group-name="my-app" daemon="false"/>
 <thread-factory name="MyThreadFactory" group="MyThreadGroup"/>
 <thread-pool-executor name="MyExecutor" thread-factory="MyThreadFactory">
 <core-pool-size count="2" per-cpu="1.2"/>
 <max-pool-size count="4" per-cpu="2"/>
 <bounded-queue size="50"/>
 <reject-policy name="block"/>
 </thread-pool-executor>
</threads>

This makes an injectable bean called "MyExecutor" which I can add to my application using the normal mechanisms.

Alternately I could "borrow" a thread pool executor declared elsewhere, with a hook so that it changes the thread's name when it's running one of my tasks:

<threads>
 <notating-executor name="MyExecutor" parent="SomeOtherExecutor" note="Running my task"/>
</threads>

There are three different queue types available (bounded, unbounded, and direct), a variety of reject-policies, and pluggable interrupt and exception handlers.

One thing I did not (yet) account for is a facility to propagate various thread context information to tasks (things like a context classloader, security context, transaction context, etc). I imagine some kind of pluggable system where a context type can be registered with code that knows how to get context from one thread and put context to a new thread. Then it's just a question of adding a plugin for each context type, and new services which keep a thread context could just be registered.

[dmlloyd]

The fix/work around we have is to stop step (8) from occuring.i.e. you never allow the thread pool to go below a minimum of one thread which avoids the race condition - there is always at least one thread available to execute the work.

Ah, I understand. I intuited that this might be the case so I made the core thread pool size be implicitly at least 1, but I agree that this is undesirable overall. It would be nice if thread pools which are never used would never produce any threads. For my "blocking" policy, I'm not actually using a special rejection handler; instead I wrap the BlockingQueue with one that blocks on "offer". I still have to do some testing to see if that's a good approach, however I suspect it will not suffice to avoid this race condition even if it is a usable approach overall.

[dmlloyd]

In regards to the empty pool vs. blocking policy race condition...

I think my solution solves the deadlock. Since the blocking occurs on the offer() rather than reject(), ThreadPoolExecutor has code in place to handle this problem (they need to handle it anyway - even if offer() works in the normal nonblocking fashion, the condition may still occur where the task is added to the queue but no threads remain to handle it). Namely, after successfully adding an element to the queue, it checks to verify that there is at least one thread around to handle the task, and if not, it starts one.

I'm in the process now of figuring out a test case that artificially creates this scenario...

[dmlloyd]

...and I've created one. It makes sure that the queue submit unblocks after the worker thread dies. The executor used in the test is basically a regular ThreadPoolExecutor with the same queue wrapper, except I wrap the wrapped queue again to add addtional blocking so that the queue unblocks right when the race would occur - when the worker thread from the first task exits leaving zero threads in the pool.

http://anonsvn.jboss.org/repos/sandbox/david.lloyd/jboss-threads/trunk/main/src/test/java/org/jboss/threads/ThreadPoolTestCase.java

[adrian.brock]

...and I've created one. It makes sure that the queue submit unblocks after the worker thread dies. The executor used in the test is basically a regular ThreadPoolExecutor with the same queue wrapper, except I wrap the wrapped queue again to add addtional blocking so that the queue unblocks right when the race would occur - when the worker thread from the first task exits leaving zero threads in the pool.

http://anonsvn.jboss.org/repos/sandbox/david.lloyd/jboss-threads/trunk/main/src/test/java/org/jboss/threads/ThreadPoolTestCase.java

You can't test a race condition by blocking.
Its a race condition because the thing is not threadsafe.
At best, all you'll do is cause a deadlock in your test. ;-)

The only way to test for a race condition is to stress test something that is
likely to reproduce the problem (in this case it is very difficult because
it requires starving a thread of cpu longer than its idle time :-)

On your solution, I don't think it resolves the real issue?

The issue is that the
* offer to the queue
* end previous runnable
* add a thread
are not synchronized with each other.

i.e. there are gaps between the addWorker() check and the offer to the queue
where the thread that addWorker() thinks exists
gets ended due to idleness leaving the Runnable in limbo.
Only a new call to addWorker() would recheck the active threads and spot
that there's no threads to execute the runnable.

So another workaround (besides the minimum of 1 threads) would be to introduce
a periodic redo of the addWorker() check.

Or put another way, whether you block in the queue.offer() or the rejection()
won't make any real difference as you can see from the openjdk execute() code
they are essentially in the same place

 if (isRunning(c) && workQueue.offer(command)) {
 int recheck = ctl.get();
 if (! isRunning(recheck) && remove(command))
 reject(command);
 else if (workerCountOf(recheck) == 0)
 addWorker(null, false);
 }
 else if (!addWorker(command, false))
 reject(command);

[dmlloyd]

Hmm. There's differences between the openjdk version and the JDK6 version...

I need to think about this some more. Maybe ThreadPoolExecutor is not something we should be using.

[dmlloyd]

After spending far too long on this problem, the solution I have come up with (this time) is to have an alternate executor implementation which is used when the configuration given is known not to work with TPE. In this case, it is chosen whenever there is a blocking policy in place, but it could be made more (or less) specific if desired.

The alternate implementation is designed to be correct and simple ("SimpleQueueExecutor"); thus it uses a Queue (rather than a BlockingQueue) to hold tasks and simply uses a Lock and Conditions to synchronize and block as needed, in order to allow for atomic "create-thread-or-queue" and "dequeue-or-destroy-thread" operations. It is designed to hold the lock for as small a time as possible (generally just long enough to do a couple field accesses, comparisons, and maybe a queue insert/remove) without making the code too complex. As such it is considerably simpler than ThreadPoolExecutor (less than 600 lines versus approx. 1800), though I won't place any bets as to comparative performance until I can figure out a way to get a somewhat reliable benchmark result. I'll consider it a runaway success if I achieve "approximately similar". In support of this class, there's also a simple bounded, array-backed queue class ("ArrayQueue") since there is (amazingly) no such thing in the JDK.

As for the common-core stuff - I think that (for the most part) the various extensions can work with this pojo-centric design, but maybe not exactly as-is (since there is some tight coupling with ThreadPoolExecutor in a few places that I see from my initial read through it all). For now I think I'll just postpone the issue - at least until I have fulfilled my immediate obligations.

Anyway, here's links to the more interesting points:

http://anonsvn.jboss.org/repos/sandbox/david.lloyd/jboss-threads/trunk/main/src/main/java/org/jboss/threads/SimpleQueueExecutor.java

http://anonsvn.jboss.org/repos/sandbox/david.lloyd/jboss-threads/trunk/jbossmc/src/main/resources/schema/jboss-threads_1_0.xsd

[dmlloyd]

So, two questions. First: Is this something that can/should be included with JBossMC? Second: If so, what are the criteria and what is the procedure for inclusion?

[alesj]

Is this something that can/should be included with JBossMC?

I see you already have a split - common vs. MC.
I would put the common into commons :-),
e.g. common-threads:
- http://anonsvn.jboss.org/repos/common/threads

And the MC part into deployers/deployers-impl.
(similar place where I put my -scanning.xml and -dependency.xml metadata)

If so, what are the criteria and what is the procedure for inclusion?

The criteria is usage/dependency, e.g. vfs based stuff goes to deployers-vfs.
Procedure is full mavenization (we already have this) + complete test coverage (parsing + actual usage).