Jeanfrancois wrote:
> Just to make sure, you propose to override those methods to isolate the
> web application classes?
I am sorry.
Maybe I should fix my words. :(
I just thought that the only means of the ThreadClassloader being able to be delegated from a user's classloader was for us to make new ClassLoader and override those methods when a user's classloader didn't find any classes.
In other words, I thought that the ThreadClassloader was not the parent of user's classloader but the delegate of a user's classloader.
--
Bongjae Chang
----- Original Message -----
From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
To: <dev_at_grizzly.dev.java.net>
Sent: Monday, August 31, 2009 11:14 PM
Subject: Re: About ServletAdapter's classloader
> Salut,
>
> Bongjae Chang wrote:
>> Hi Jeanfrancois,
>>
>> Jeanfrancois wrote:
>>> 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?
>>>
>>
>> I understood your word just now. You are right.
>>
>> Current ServletAdapter to which I applied the patch doesn't have your classloader hierarchy.
>>
>> The current classloader hierarchy is here.
>> --------
>> ServletAdapterA_CL's parent ServletAdapterB_CL's parent
>> / \
>> ServletAdapterA_CL ServletAdapterB_CL
>> --------
>>
>> But, I also think that the parent classloader should be not their own Classloader's parent CL but common ThreadClassloader.
>
> Agree.
>
>>
>> For your classloader hierarchy, maybe I think that we should make new ClassLoader and should override ContextLoader#loadClass() and findClass().
>>
>> Am I understanding it correctly?
>
> Just to make sure, you propose to override those methods to isolate the
> web application classes?
>
> Thanks!
>
> -- Jeanfrancois
>
>
>>
>> --
>> 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
>>>
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>
>
>
>