Thank you for your reply, Jeanfrancois.
And Hubert, please let me know your thoughts. :)
Thanks!
--
Bongjae Chang
----- Original Message -----
From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
To: <dev_at_grizzly.dev.java.net>
Sent: Saturday, August 29, 2009 2:41 AM
Subject: Re: About ServletAdapter's classloader
> 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
>>>
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>
>
>
>