Hi,
I know current DefaultThreadPool is modified from PipelineThreadPool and original DefaultThreadPool is renamed to JDKThreadPool at Rev.3265.
But I think that some logic needs more.
Original DefaultThreadPool had beforeExecute() and afterExecute() like this.
---
protected void beforeExecute(Thread t, Runnable r) {
((WorkerThreadImpl) t).createByteBuffer(false);
}
protected void afterExecute(Runnable r, Throwable t ) {
((WorkerThreadImpl) Thread.currentThread()).reset();
}
---
But current DefaultThreadPool doesn't have them.
So sometimes I could see the following error when a message was being parsed.
---
java.lang.NullPointerException
at com.sun.enterprise.mgmt.transport.grizzly.GrizzlyMessageProtocolParser.startBuffer(GrizzlyMessageProtocolParser.java:158)
at com.sun.grizzly.filter.ParserProtocolFilter.execute(ParserProtocolFilter.java:134)
at com.sun.grizzly.DefaultProtocolChain.executeProtocolFilter(DefaultProtocolChain.java:135)
at com.sun.grizzly.DefaultProtocolChain.execute(DefaultProtocolChain.java:102)
at com.sun.grizzly.DefaultProtocolChain.execute(DefaultProtocolChain.java:88)
at com.sun.grizzly.DefaultCallbackHandler.onRead(DefaultCallbackHandler.java:149)
at com.sun.grizzly.DefaultCallbackHandler.onWrite(DefaultCallbackHandler.java:163)
at com.sun.grizzly.CallbackHandlerContextTask.doCall(CallbackHandlerContextTask.java:66)
at com.sun.grizzly.SelectionKeyContextTask.call(SelectionKeyContextTask.java:57)
at com.sun.grizzly.ContextTask.run(ContextTask.java:69)
at com.sun.grizzly.util.FixedThreadPool$BasicWorker.dowork(FixedThreadPool.java:338)
at com.sun.grizzly.util.FixedThreadPool$BasicWorker.run(FixedThreadPool.java:321)
at java.lang.Thread.run(Thread.java:619)
---
Here is GrizzlyMessageProtocolParser.java's 158 line.
---
public void startBuffer( ByteBuffer bb ) {
this.byteBuffer = bb;
if( byteBuffer.capacity() < MessageImpl.MAX_TOTAL_MESSAGE_LENGTH ) { // 158 line
if( LOG.isLoggable( Level.WARNING ) )
LOG.log( Level.WARNING, "byte buffer capacity is too small: capacity = " + byteBuffer.capacity() );
}
}
---
It seems that ByteBuffer is null. As you know, ProtocolParser#startBuffer(ByteBuffer bb) is called by ParserProtocolFilter#execute().
The ByteBuffer is same to WorkerThread's byteBuffer.
I think that this may be a side-effect between rev.3265 and Grizzly issue #605(
https://grizzly.dev.java.net/issues/show_bug.cgi?id=605, " Eliminating the byte buffer's allocation in WorkerThread#detach()")
So I think that it is better that FixedThreadPool supports beforeExecute() and afterExecute() like JDK's thread pool.
What do you think?
I attached the proposed patch.
Thanks.
Index: main/java/com/sun/grizzly/util/DefaultThreadPool.java
===================================================================
--- main/java/com/sun/grizzly/util/DefaultThreadPool.java (revision 3293)
+++ main/java/com/sun/grizzly/util/DefaultThreadPool.java (working copy)
@@ -343,6 +343,16 @@
sb.append(", is-shutdown=").append(isShutdown());
}
+ @Override
+ protected void beforeExecute(Thread t, Runnable r) {
+ ((WorkerThreadImpl) t).createByteBuffer(false);
+ }
+
+ @Override
+ protected void afterExecute(Runnable r, Throwable t) {
+ ((WorkerThreadImpl) Thread.currentThread()).reset();
+ }
+
private class DefaultWorkerThreadFactory implements ThreadFactory {
public Thread newThread(Runnable r) {
Thread thread = new WorkerThreadImpl(DefaultThreadPool.this,
Index: main/java/com/sun/grizzly/util/FixedThreadPool.java
===================================================================
--- main/java/com/sun/grizzly/util/FixedThreadPool.java (revision 3293)
+++ main/java/com/sun/grizzly/util/FixedThreadPool.java (working copy)
@@ -309,7 +309,46 @@
return threadFactory;
}
-
+ /**
+ * Method invoked prior to executing the given Runnable in the
+ * given thread. This method is invoked by thread <tt>t</tt> that
+ * will execute task <tt>r</tt>, and may be used to re-initialize
+ * ThreadLocals, or to perform logging.
+ *
+ * <p>This implementation does nothing, but may be customized in
+ * subclasses. Note: To properly nest multiple overridings, subclasses
+ * should generally invoke <tt>super.beforeExecute</tt> at the end of
+ * this method.
+ *
+ * @param t the thread that will run task r.
+ * @param r the task that will be executed.
+ */
+ protected void beforeExecute(Thread t, Runnable r) { }
+
+ /**
+ * Method invoked upon completion of execution of the given Runnable.
+ * This method is invoked by the thread that executed the task. If
+ * non-null, the Throwable is the uncaught <tt>RuntimeException</tt>
+ * or <tt>Error</tt> that caused execution to terminate abruptly.
+ *
+ * <p><b>Note:</b> When actions are enclosed in tasks (such as
+ * {_at_link java.util.concurrent.FutureTask}) either explicitly or via methods such as
+ * <tt>submit</tt>, these task objects catch and maintain
+ * computational exceptions, and so they do not cause abrupt
+ * termination, and the internal exceptions are <em>not</em>
+ * passed to this method.
+ *
+ * <p>This implementation does nothing, but may be customized in
+ * subclasses. Note: To properly nest multiple overridings, subclasses
+ * should generally invoke <tt>super.afterExecute</tt> at the
+ * beginning of this method.
+ *
+ * @param r the runnable that has completed.
+ * @param t the exception that caused termination, or null if
+ * execution completed normally.
+ */
+ protected void afterExecute(Runnable r, Throwable t) { }
+
protected class BasicWorker implements Runnable{
Thread t;
@@ -333,10 +372,15 @@
if (r == poison || r == null){
return;
}
+ boolean ran = false;
+ beforeExecute( t, r );
try {
approximateRunningWorkerCount.incrementAndGet();
r.run();
+ afterExecute( r, null );
} catch( Throwable t ) {
+ if (!ran)
+ afterExecute( r, t );
throw t;
} finally {
approximateRunningWorkerCount.decrementAndGet();
--
Bongjae Chang