On Tue, Jun 28, 2016 at 12:37 PM, Rainer Jung <rainer.j...@kippdata.de> wrote:
> Am 28.06.2016 um 18:06 schrieb therealnewo...@gmail.com:
>>
>> On Tue, Jun 28, 2016 at 11:51 AM, Rainer Jung <rainer.j...@kippdata.de>
>> wrote:
>>>
>>> Am 28.06.2016 um 16:07 schrieb Mark Thomas:
>>>>
>>>>
>>>> On 28/06/2016 12:28, Mark Thomas wrote:
>>>>>
>>>>>
>>>>> On 28/06/2016 11:34, Rainer Jung wrote:
>>>>>>
>>>>>>
>>>>>> Am 28.06.2016 um 11:15 schrieb Mark Thomas:
>>>>>
>>>>>
>>>>>
>>>>> <snip />
>>>>>
>>>>>>> Index: src/ssl.c
>>>>>>> ===================================================================
>>>>>>> --- src/ssl.c (revision 1750259)
>>>>>>> +++ src/ssl.c (working copy)
>>>>>>> @@ -420,6 +420,10 @@
>>>>>>> return psaptr->PSATOLD;
>>>>>>> #elif defined(WIN32)
>>>>>>> return (unsigned long)GetCurrentThreadId();
>>>>>>> +#elif defined(DARWIN)
>>>>>>> + uint64_t tid;
>>>>>>> + pthread_threadid_np(NULL, &tid);
>>>>>>> + return (unsigned long)tid;
>>>>>>> #else
>>>>>>> return (unsigned long)(apr_os_thread_current());
>>>>>>> #endif
>>>>>>>
>>>>>>>
>>>>>>> I want to do some similar testing for Linux before adding what I
>>>>>>> suspect
>>>>>>> will be a very similar block using gettid().
>>>>>>
>>>>>>
>>>>>>
>>>>>> We could either add something to configure.in. Untested:
>>>>>>
>>>>>> Index: native/configure.in
>>>>>> ===================================================================
>>>>>> --- native/configure.in (revision 1750462)
>>>>>> +++ native/configure.in (working copy)
>>>>>> @@ -218,6 +218,9 @@
>>>>>> *-solaris2*)
>>>>>> APR_ADDTO(TCNATIVE_LIBS, -lkstat)
>>>>>> ;;
>>>>>> + *linux*)
>>>>>> + APR_ADDTO(CFLAGS, -DTCNATIVE_LINUX)
>>>>>> + ;;
>>>>>> *)
>>>>>> ;;
>>>>>> esac
>>>>>>
>>>>>>
>>>>>> and then use a
>>>>>>
>>>>>> #ifdef TCNATIVE_LINUX
>>>>>>
>>>>>> or we copy some other more direct linux check from e.g. APR:
>>>>>>
>>>>>> #ifdef __linux__
>>>>>>
>>>>>> The latter looks simpler, but the version above is based on all the
>>>>>> logic put into config.guess.
>>>>>
>>>>>
>>>>>
>>>>> I'd go with the __linux__ option as that is consistent with what we
>>>>> already use in os/unix/system.c
>>>>>
>>>>> I'm not against the change to configure.in, I just think we should be
>>>>> consistent with how we do this throughout the code base.
>>>>
>>>>
>>>>
>>>> I've confirmed that the same problem occurs with hash bucket selection
>>>> on linux and that switching to gettid() fixes that problem.
>>>>
>>>> I'm going to go ahead with the 1.2.8 release shortly. We can continue to
>>>> refine this as necessary and have a more complete fix in 1.2.9.
>>>
>>>
>>>
>>> I did a quick check on Solaris. apr_os_thread_current() uses pthread_self
>>> on
>>> Solaris like on Linux (actually on any Unix type OS), but unlike Linux
>>> where
>>> this returns a address which is either 32 or 64 bit aligned depending on
>>> address size, on Solaris you get an increasing number starting with 1 for
>>> the first thread and incremented by one for each following thread. Thread
>>> IDs do not get reused in the same process, even if the thread finished,
>>> but
>>> thread IDs are common between different processes, because they always
>>> start
>>> with 1. So Solaris should be fine as-is.
>>
>>
>> Does the value have a cap? If not then Solaris will just continue to
>> use more and more memory as threads are created over the lifetime of
>> the server.
>
>
> No cap. I think everyone agrees we are still looking at cleaning up at
> thread death as a further improvement. Whether there's a problem without it
> depends on the executor size and whether the executor actually destroys and
> recreates threads (thus using more IDs than the max thread count).
>
Mark,
The change which was added to 1.2.8 works great on 1.1.34. I also
hacked in calling ERR_remove_thread_state on thread exit. It is
similar to my original change but only does do it on a threads exit,
but as Remy pointed out earlier this will only work if all IO
operations occur on one of the original threads from the thread pool.
-nate
Here is the hack just for completeness.
diff --git a/java/org/apache/catalina/core/StandardThreadExecutor.java
b/java/org/apache/catalina/core/StandardThreadExecutor.java
index a30f29c..80f429d 100644
--- a/java/org/apache/catalina/core/StandardThreadExecutor.java
+++ b/java/org/apache/catalina/core/StandardThreadExecutor.java
@@ -18,6 +18,7 @@
package org.apache.catalina.core;
import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import org.apache.catalina.Executor;
@@ -27,10 +28,11 @@ import org.apache.catalina.util.LifecycleMBeanBase;
import org.apache.tomcat.util.threads.ResizableExecutor;
import org.apache.tomcat.util.threads.TaskQueue;
import org.apache.tomcat.util.threads.TaskThreadFactory;
+import org.apache.tomcat.util.threads.ThreadCreator;
import org.apache.tomcat.util.threads.ThreadPoolExecutor;
public class StandardThreadExecutor extends LifecycleMBeanBase
- implements Executor, ResizableExecutor {
+ implements Executor, ResizableExecutor, ThreadCreator {
// ---------------------------------------------- Properties
/**
@@ -319,6 +321,10 @@ public class StandardThreadExecutor extends
LifecycleMBeanBase
return false;
}
+ @Override
+ public ThreadFactory getThreadFactory() {
+ return executor.getThreadFactory();
+ }
@Override
protected String getDomainInternal() {
diff --git a/java/org/apache/tomcat/jni/SSL.java
b/java/org/apache/tomcat/jni/SSL.java
index b726a54..fb9fc16 100644
--- a/java/org/apache/tomcat/jni/SSL.java
+++ b/java/org/apache/tomcat/jni/SSL.java
@@ -388,4 +388,10 @@ public final class SSL {
* @return true if all SSL_OP_* are supported by OpenSSL library.
*/
public static native boolean hasOp(int op);
+
+ /**
+ * Cleanup thread specific data structures. This needs to be
called after a thread
+ * has completed ssl operations.
+ */
+ public static native void threadCleanup();
}
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java
b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 74d813f..2edf673 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -29,6 +29,7 @@ import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.Executor;
import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.juli.logging.Log;
@@ -50,6 +51,8 @@ import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.net.AbstractEndpoint.Acceptor.AcceptorState;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
import org.apache.tomcat.util.security.PrivilegedSetTccl;
+import org.apache.tomcat.util.threads.TaskThreadFactory;
+import org.apache.tomcat.util.threads.ThreadCreator;
/**
@@ -670,6 +673,14 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
createExecutor();
}
+ Executor executor = getExecutor();
+ if (isSSLEnabled() && executor instanceof ThreadCreator) {
+ ThreadFactory tf = ((ThreadCreator)
executor).getThreadFactory();
+ if (tf instanceof TaskThreadFactory) {
+ ((TaskThreadFactory) tf).addCleanupTask(SSL_CLEANUP);
+ }
+ }
+
initializeConnectionLatch();
// Start poller thread
@@ -1024,6 +1035,13 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
return log;
}
+ private static final Runnable SSL_CLEANUP = new Runnable() {
+ @Override
+ public void run() {
+ SSL.threadCleanup();
+ }
+ };
+
// --------------------------------------------------- Acceptor Inner Class
/**
* The background thread that listens for incoming TCP/IP connections and
diff --git a/java/org/apache/tomcat/util/threads/TaskThreadFactory.java
b/java/org/apache/tomcat/util/threads/TaskThreadFactory.java
index af63f11..12a4eb7 100644
--- a/java/org/apache/tomcat/util/threads/TaskThreadFactory.java
+++ b/java/org/apache/tomcat/util/threads/TaskThreadFactory.java
@@ -16,6 +16,8 @@
*/
package org.apache.tomcat.util.threads;
+import java.util.Collection;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicInteger;
/**
@@ -29,6 +31,8 @@ public class TaskThreadFactory implements ThreadFactory {
private final String namePrefix;
private final boolean daemon;
private final int threadPriority;
+ private final Collection<Runnable> cleanupTasks = new
CopyOnWriteArrayList<Runnable>();
+
public TaskThreadFactory(String namePrefix, boolean daemon, int priority) {
SecurityManager s = System.getSecurityManager();
group = (s != null) ? s.getThreadGroup() :
Thread.currentThread().getThreadGroup();
@@ -38,11 +42,36 @@ public class TaskThreadFactory implements ThreadFactory {
}
@Override
- public Thread newThread(Runnable r) {
- TaskThread t = new TaskThread(group, r, namePrefix +
threadNumber.getAndIncrement());
+ public Thread newThread(final Runnable r) {
+ Runnable wrapped = new Runnable() {
+ @Override
+ public void run() {
+ try {
+ r.run();
+ } finally {
+ for (Runnable task : cleanupTasks) {
+ task.run();
+ }
+ }
+ }
+ };
+ TaskThread t = new TaskThread(group, wrapped, namePrefix +
threadNumber.getAndIncrement());
t.setDaemon(daemon);
t.setPriority(threadPriority);
return t;
}
+ public void addCleanupTask(Runnable task) {
+ synchronized (cleanupTasks) {
+ if (!cleanupTasks.contains(task)) {
+ cleanupTasks.add(task);
+ }
+ }
+ }
+
+ public boolean removeCleanupTask(Runnable task) {
+ synchronized (cleanupTasks) {
+ return cleanupTasks.remove(task);
+ }
+ }
}
diff --git a/java/org/apache/tomcat/util/threads/ThreadCreator.java
b/java/org/apache/tomcat/util/threads/ThreadCreator.java
new file mode 100644
index 0000000..94270b5
--- /dev/null
+++ b/java/org/apache/tomcat/util/threads/ThreadCreator.java
@@ -0,0 +1,17 @@
+/*
+ * ========================================================================
+ *
+ * Copyright (c) by Hitachi Data Systems, 2016. All rights reserved.
+ *
+ * ========================================================================
+ */
+package org.apache.tomcat.util.threads;
+
+import java.util.concurrent.ThreadFactory;
+
+public interface ThreadCreator {
+ /**
+ * @return The {@link ThreadFactory} used by this instance
+ */
+ ThreadFactory getThreadFactory();
+}
diff --git a/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
b/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
index bc84ad3..1479783 100644
--- a/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
+++ b/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
@@ -34,7 +34,7 @@ import org.apache.tomcat.util.res.StringManager;
* @author fhanik
*
*/
-public class ThreadPoolExecutor extends
java.util.concurrent.ThreadPoolExecutor {
+public class ThreadPoolExecutor extends
java.util.concurrent.ThreadPoolExecutor implements ThreadCreator {
/**
* The string manager for this package.
*/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
