Salut,
Bongjae Chang wrote:
> Hi Jeanfrancois,
>
> Bongjae wrote:
>>> C) But I think that this way is not safe because we can't estimate
>>> when new thread will be created and what is current thread's ContextLoader.
>
> Jeanfrancois wrote:
>> But Should all the newly created Thread share the *same*
>> ContextClassLoader? The ServletAdapter must have its own, but the parent
>> should have the same IMO.
>
> Maybe I didn't understand your question correctly.
>
> Could you please explain more?
I think what we want is
ThreadClassloader
/ \
ServletAdapterA_CL ServletAdapterB_CL
two ServletAdapter instance will have their own Classloader referencing
their web-inf/lib or classes directory, but they will share a common
based. Does it help?
>
>
> Bongjae wrote:
>>> But, I think that maybe this B)'s patch can have many regressions
>>> if user's adapters(custom adapters) doesn't consider their classloader.
>
> Jeanfrancois wrote:
>> Can you elaborate about that one? I don't see where your two patches
>> could conflict. IMO they are needed both.
>
> Right. Maybe I was misunderstanding something yesterday. Please forget my words. :)
>
>
> I know that grizzly deployer refactoring is in progress recently(It is great!). When I saw new grizzly deployer simply, it seemed that the WebAppAdapter would resolve this issue.
>
> Though, do you think that it is better that my patch should be applyed to current version?
Yes I would think so. Hubert, what do you think? We can merge later
unless you thunk it will breaks your work.
>
> If yes, I will open two issue trackers(1. About ServletAdapter's own classloader, 2. About HttpWorkerThreadFactory's setContextClassLoader()) and commit them.
>
> Please let me know.
+1
Thanks
-- Jeanfrancois
>
> Thanks!
>
> --
> Bongjae Chang
>
>
> ----- Original Message -----
> From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
> To: <dev_at_grizzly.dev.java.net>
> Sent: Friday, August 28, 2009 12:25 AM
> Subject: Re: About ServletAdapter's classloader
>
>
>> Salut,
>>
>> Bongjae Chang wrote:
>>> Hi,
>>>
>>> When I tested GrizzlyWebServer and GrizzlyWebServerDeployer variously, I
>>> found some interesting points.
>>>
>>> When I deployed an web application after GrizzlyWebServer.start(),
>>> sometimes the web application didn't work well.
>>>
>>> The problem was that my web application didn't access own resources.
>>> (Many web applications used to load resources from current thread's
>>> ContextLoader)
>>>
>>> But, when I deployed an web application after GrizzlyWebServer.start()
>>> was called, it worked fine.
>>>
>>> When I debugged it, it seemed that this problem had been caused by
>>> current thread's ContextLoader.
>>>
>>> When GrizzlyWebServer(gws) was started, the gws initialized the thread
>>> pool. (See the SelectorThread#initThreadPool() ).
>>>
>>> Default thread pool is StatsThreadPool and default ThreadFactory is
>>> HttpWorkerThreadFactory.
>>>
>>> When HttpWorkerThreadFactory creates new thread, HttpWorkerThreadFactory
>>> doesn't set thread's ContextClassLoader.
>> That's an issue.
>>
>>>
>>> Then, threads of the thread pool will use the class loader where
>>> gws.start() is executed as ContextClassLoader because the
>>> ContextClassLoader is propagated from the parent thread when new thread
>>> is created.
>>>
>>> And the threads' ContextClassLoader will be served when clients request
>>> the service.
>>>
>>> So I understood my problems. Actually when I met errors in my test, the
>>> serviced ContextClassLoader(Thread.currentThread().getContextClassLoader())
>>> is not the application class loader which I created.
>>>
>>> If users would like to deploy applications like using
>>> gws.addGrizzlyAdapter() after gws has started, it seems that current
>>> grizzly can't support them correctly because the adapter doesn't have
>>> own classloader.
>>>
>>> A) So, I think that it is better that the adapter has own classloader.
>> +1
>>
>>>
>>> Moreover, when I used GrizzlyWebServerDeployer, I could see the
>>> duplicated classloader hierarchy.
>>>
>>> For examples,
>>>
>>> Assuming that the "example" web application has the following libraries,
>>> ---
>>> /example/WEB-INF/lib/a.jar
>>> /example/WEB-INF/lib/b.jar
>>> /example/WEB-INF/classes/c.class
>>> /example/WEB-INF/classes/d.class
>>> ---
>>>
>>> GrizzlyWebServerDeploy will make new URLClassLoader*(1)* which has
>>> a.jar, b.jar, c.class and d.class before GrizzlyWebServerDeployer
>>> deploys the app and set the URLClassLoader to be current thread's
>>> ContextLoader.
>>>
>>> Then, an ServletAdaper will be added to GrizzlyWebServer and the adapter
>>> will be started. See the following code.
>>>
>>> In ServletAdapter.java
>>> ---
>>> public void start() {
>>> try{
>>> if (initialize) {
>>> initWebDir();
>>> configureClassLoader(webDir.getCanonicalPath());
>>> }
>>> } catch(...) {
>>> ...
>>> }
>>> }
>>>
>>> protected void configureClassLoader(String applicationPath) throws
>>> IOException{
>>>
>>> Thread.currentThread().setContextClassLoader(ClassLoaderUtil.createURLClassLoader(applicationPath));
>>> --- *(2)*
>>> }
>>> ---
>>>
>>> GrizzlyWebServerDeploy already set the application classloader but
>>> ServletAdapter always creates new URLClassLoader with
>>> ClassLoaderUtil.createURLClassLoader().
>>>
>>> Then, the classloader hierarchy is here.
>>> ---
>>> GrizzlyWebServerDeployer's app classloader *(1)* --- parent classloader
>>> |
>>> ServletAdaper's app classloader *(2)*
>>> ---
>>>
>>> (1) and (2) classloaders have same class path.
>>>
>>>
>>> B) So, I think that if the ServletAdapter already has the app
>>> classloader explicitly, ServletAdapter need not to create new app
>>> classloader.
>> Agree.
>>
>>>
>>>
>>> Here is the my example patch about (A) and (B).
>>>
>>> Index: com/sun/grizzly/http/servlet/ServletAdapter.java
>>> ===================================================================
>>> --- com/sun/grizzly/http/servlet/ServletAdapter.java (revision 3546)
>>> +++ com/sun/grizzly/http/servlet/ServletAdapter.java (working copy)
>>> @@ -172,6 +172,8 @@
>>> * Initialize the {_at_link <mailto:{_at_link> ServletContext}
>>> */
>>> protected boolean initialize = true;
>>> +
>>> + protected ClassLoader classLoader;
>>>
>>>
>>> public ServletAdapter() {
>>> @@ -274,11 +276,26 @@
>>> initWebDir();
>>> configureClassLoader(webDir.getCanonicalPath());
>>> }
>>> - configureServletEnv();
>>> - fullUrlPath = contextPath + servletPath;
>>> - setResourcesContextPath(fullUrlPath);
>>> - if (loadOnStartup){
>>> - loadServlet();
>>> + if( classLoader != null ) {
>>> + ClassLoader prevClassLoader =
>>> Thread.currentThread().getContextClassLoader();
>>> + Thread.currentThread().setContextClassLoader(
>>> classLoader );
>>> + try {
>>> + configureServletEnv();
>>> + fullUrlPath = contextPath + servletPath;
>>> + setResourcesContextPath( fullUrlPath );
>>> + if( loadOnStartup ) {
>>> + loadServlet();
>>> + }
>>> + } finally {
>>> + Thread.currentThread().setContextClassLoader(
>>> prevClassLoader );
>>> + }
>>> + } else {
>>> + configureServletEnv();
>>> + fullUrlPath = contextPath + servletPath;
>>> + setResourcesContextPath( fullUrlPath );
>>> + if( loadOnStartup ) {
>>> + loadServlet();
>>> + }
>>> }
>>> } catch (Throwable t){
>>> logger.log(Level.SEVERE,"start",t);
>>> @@ -294,8 +311,8 @@
>>> * @throws java.io.IOException I/O error.
>>> */
>>> protected void configureClassLoader(String applicationPath) throws
>>> IOException{
>>> - Thread.currentThread().setContextClassLoader(
>>> - ClassLoaderUtil.createURLClassLoader(applicationPath));
>>> + if( classLoader == null )
>>> + classLoader =
>>> ClassLoaderUtil.createURLClassLoader(applicationPath);
>>> }
>>>
>>>
>>> @@ -304,6 +321,20 @@
>>> */
>>> @Override
>>> public void service(GrizzlyRequest request, GrizzlyResponse response) {
>>> + if( classLoader != null ) {
>>> + ClassLoader prevClassLoader =
>>> Thread.currentThread().getContextClassLoader();
>>> + Thread.currentThread().setContextClassLoader( classLoader );
>>> + try {
>>> + doService( request, response );
>>> + } finally {
>>> + Thread.currentThread().setContextClassLoader(
>>> prevClassLoader );
>>> + }
>>> + } else {
>>> + doService( request, response );
>>> + }
>>> + }
>>> +
>>> + public void doService(GrizzlyRequest request, GrizzlyResponse
>>> response) {
>>> try {
>>> Request req = request.getRequest();
>>> Response res = response.getResponse();
>>> @@ -679,9 +710,21 @@
>>> */
>>> @Override
>>> public void destroy(){
>>> - super.destroy();
>>> - servletCtx.destroyListeners();
>>> - filterChain.destroy();
>>> + if( classLoader != null ) {
>>> + ClassLoader prevClassLoader =
>>> Thread.currentThread().getContextClassLoader();
>>> + Thread.currentThread().setContextClassLoader( classLoader );
>>> + try {
>>> + super.destroy();
>>> + servletCtx.destroyListeners();
>>> + filterChain.destroy();
>>> + } finally {
>>> + Thread.currentThread().setContextClassLoader(
>>> prevClassLoader );
>>> + }
>>> + } else {
>>> + super.destroy();
>>> + servletCtx.destroyListeners();
>>> + filterChain.destroy();
>>> + }
>>> }
>>>
>>>
>>> @@ -697,6 +740,8 @@
>>> ServletAdapter sa = new ServletAdapter(".",servletCtx,
>>> contextParameters,
>>> new HashMap<String,String>(), listeners,
>>> false);
>>> + if( classLoader != null )
>>> + sa.setClassLoader( classLoader );
>>> sa.setServletInstance(servlet);
>>> sa.setServletPath(servletPath);
>>> return sa;
>>> @@ -713,4 +758,12 @@
>>> protected HashMap<String, String> getContextParameters() {
>>> return contextParameters;
>>> }
>>> +
>>> + public ClassLoader getClassLoader() {
>>> + return classLoader;
>>> + }
>>> +
>>> + public void setClassLoader( ClassLoader classLoader ) {
>>> + this.classLoader = classLoader;
>>> + }
>>> }
>>>
>>>
>>> Then the adapter will use the user's classloader instead of thread
>>> pool's classloader when a user creates an adapter like this.
>> So far I really like your patch!
>>
>>
>>>
>>> ---
>>> ServletAdapter sa = new ServletAdapter();
>>> *sa.setClassLoader( userClassLoader );*
>>> sa.setContextPath( contextPath );
>>> sa.setServletPath( servletPath );
>>> sa.setRootFolder( rootFolder );
>>> ...
>>> ---
>>>
>>> If an user doesn't set the classloader of ServletAdapter, I think that
>>> the behavior will be similar to old version's behavior.
>> Agree.
>>
>>
>>>
>>>
>>>
>>> Another issue is about HttpWorkerThreadFactory.
>>>
>>> Current HttpWorkerThreadFactory doesn't set the ContextClassLoader. Then
>>> the current ContextClassLoader of executing the thread pool will be used
>>> when the thread pool creates new threads as I said above.
>>>
>>> C) But I think that this way is not safe because we can't estimate
>>> when new thread will be created and what is current thread's ContextLoader.
>> But Should all the newly created Thread share the *same*
>> ContextClassLoader? The ServletAdapter must have its own, but the parent
>> should have the same IMO.
>>
>>
>>>
>>> If current thread's ContextLoader is user's application classloader,
>>> memory leak can be occurred because the thread pool can preserve the
>>> classloader after the application already undeployed. (But, I am not sure)
>> Yes usually I saw that issue with Tomcat and GlassFish...many many times.
>>
>>>
>>> So, I think that the following patch is better about C).
>>>
>>> Index: com/sun/grizzly/http/StatsThreadPool.java
>>> ===================================================================
>>> --- com/sun/grizzly/http/StatsThreadPool.java (revision 3546)
>>> +++ com/sun/grizzly/http/StatsThreadPool.java (working copy)
>>> @@ -41,6 +41,8 @@
>>> import java.util.concurrent.LinkedBlockingQueue;
>>> import java.util.concurrent.ThreadFactory;
>>> import java.util.concurrent.TimeUnit;
>>> +import java.security.AccessController;
>>> +import java.security.PrivilegedAction;
>>>
>>> /**
>>> * Internal FIFO used by the Worker Threads to pass information
>>> @@ -116,14 +118,21 @@
>>> * Create new {_at_link <mailto:{_at_link> HttpWorkerThread}.
>>> */
>>> protected class HttpWorkerThreadFactory implements ThreadFactory {
>>> - public Thread newThread(Runnable r) {
>>> - Thread thread = new HttpWorkerThread(StatsThreadPool.this,
>>> - name + port + "-WorkerThread(" +
>>> - workerThreadCounter.getAndIncrement() + ")", r,
>>> - initialByteBufferSize);
>>> - thread.setUncaughtExceptionHandler(StatsThreadPool.this);
>>> - thread.setPriority(priority);
>>> - return thread;
>>> + public Thread newThread(final Runnable r) {
>>> + return AccessController.doPrivileged(
>>> + new PrivilegedAction<Thread>() {
>>> + public Thread run() {
>>> + Thread thread = new HttpWorkerThread(
>>> StatsThreadPool.this,
>>> + name
>>> + port + "-WorkerThread(" +
>>> +
>>> workerThreadCounter.getAndIncrement() + ")", r,
>>> +
>>> initialByteBufferSize );
>>> + thread.setUncaughtExceptionHandler(
>>> StatsThreadPool.this );
>>> + thread.setPriority( priority );
>>> + thread.setContextClassLoader(
>>> HttpWorkerThreadFactory.class.getClassLoader() );
>>> + return thread;
>>> + }
>>> + }
>>> + );
>>> }
>>> }
>>>
>>> But, I think that maybe this B)'s patch can have many regressions
>>> if user's adapters(custom adapters) doesn't consider their classloader.
>> Can you elaborate about that one? I don't see where your two patches
>> could conflict. IMO they are needed both.
>>
>>>
>>> And the A)'s patch only considers the ServletAdapter, but GrizzlyAdapter
>>> and StaticResourcesAdapter can also have same issues.( I am not sure).
>> Both above class don't need to load classes...so I would think we are
>> safe IMO.
>>
>>>
>>> Thank you for reading my long mail contents!
>> Quite good analysis as usual! Send more analysis like that one!
>>
>> A+
>>
>> --Jeanfrancois
>>
>>>
>>> --
>>> Bongjae Chang
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>
>>
>>
>>