I've opened https://bz.apache.org/bugzilla/show_bug.cgi?id=59797 to
track this.
Nate, if you let me have your public IP, I'll get you unbanned from BZ
as well.
Mark
On 01/07/2016 15:33, Nate Clark wrote:
> 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
