Salut,
Bongjae Chang wrote:
> 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.
Right, except some classes might need to be shared like library. In that
case those file could be loaded only once by having a shared parent
classloader.
Does that help?
Thanks
-- Jeanfrancois
>
> --
> 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
>>
>>
>>
>>